Everything
php
Learn Community Releases The PHP Project
Home » Project » Newsgroups » php.internals » [RFC][DISCUSSION] Revisit trailing commas in function arguments »

[RFC][DISCUSSION] Revisit trailing commas in function arguments

php.internals

Sammy Kaye Powers

10 years ago
Hello internals friends! I'd like to open a discussion on the RFC to allow trailing commas in function arguments. https://wiki.php.net/rfc/revisit-trailing-comma-function-args Discuss! :) Thanks, Sammy Kaye Powers sammyk.me

Andrew Faulds

10 years ago
Hey Sammy, Sammy Kaye Powers wrote:
> I'd like to open a discussion on the RFC to allow trailing commas in > function arguments. > > https://wiki.php.net/rfc/revisit-trailing-comma-function-args > > Discuss! :)
Hmm. I'm not sure if this is a good idea. For arrays, trailing commas make perfect sense: arrays are very often written over multiple lines, where trailing commas look pretty, and it's likely new elements will be added or old elements removed, justifying the trailing comma for cleaner diffs and less pain when editing the source. But for functions? Most function calls are just a single line. Most function declarations, too. And even when they are multi-line, you're not going to be adding a new argument or parameter, respectively, very often. Since these additions aren't very common, you'd need to use trailing commas everywhere (yuck) to get any benefit, because you probably can't predict which instances you're going to change soon. To me this looks like something that adds another big stylistic difference to disagree on, and adds potential ugliness (`foo($bar,);`) for very limited gain. So, my intial reaction would be a -1, sorry. Thanks.
-- Andrea Faulds http://ajf.me/

Daniel Persson

10 years ago
I'll echo Andrea remarks, -1, sorry. On Thu, Oct 15, 2015, 01:25 Andrea Faulds <ajf@ajf.me> wrote:

Björn Larsson

10 years ago
Den 2015-10-14 kl. 21:25, skrev Sammy Kaye Powers:
> Hello internals friends! > > I'd like to open a discussion on the RFC to allow trailing commas in > function arguments. > > https://wiki.php.net/rfc/revisit-trailing-comma-function-args > > Discuss! :) > > Thanks, > Sammy Kaye Powers > sammyk.me >
Given the reason against this RFC in the thread it would be interesting to know why HHVM decided to implement it? Regards //Björn

Alex Munkelwitz

10 years ago
Just my own opinion, but I support this idea. For most functions it may be out of place, but when dealing with variadic functions it can add consistency to code. In addition it can make for cleaner diffs when overriding or switching back to default arguments. It is nice to not need to adjust other lines of code when removing one. Those are pretty minor gains, but I think they outweigh the minor code cruft that this could introduce. I appreciate that php allows trailing commas in arrays and I don't think there's a compelling reason to avoid making functions consistent with that behavior. -alex On Wed, Oct 14, 2015 at 11:59 PM, Björn Larsson <bjorn.x.larsson@telia.com> wrote:

Sara Golemon

10 years ago
On Wed, Oct 14, 2015 at 11:59 PM, Björn Larsson <bjorn.x.larsson@telia.com> wrote:
> Given the reason against this RFC in the thread it would be interesting > to know why HHVM decided to implement it? >
Happy to answer, but I need to state a couple things first: * I don't really care if this change lands. I'd kinda like it, but it's not solving a massive problem for me. * There aren't any compelling reasons against this. The only reason given of any note that I've seen is: "There are no compelling reasons in favor of it." And I'll agree with that. Like I just said, it's not solving any major problems, and it's not going to cause any major problems. It's just a tiny, vanishingly insignificant piece of syntactic sugar which disappears before we even get to the AST. So again, could scarcely care less, so don't expect me to champion either side, but you asked "why", so here it is: It makes code reviews marginally less ugly. That's it. It's a tiny problem to solve, and likely saves less than 100ms during diff reviews, but it's a solution to a problem. Yes, it's a problem which countless developers live with to no significant negative consequence. Solo developers and small shops won't care about this since they tend to not bother with code reviews. FB has enough engineers working on its very large codebase though, that nobody has it all paged in, so code reviews are mandatory, and if we can spend five minutes of effort to loosen the parser rules in exchange for saving 1/10th of a second on every diff review that extends/shrinks a function call/signature with no overhead, then of course we would. That's a fair exchange. Apologies if you were hoping for a compelling reason. -Sara

Björn Larsson

10 years ago
Den 2015-10-15 kl. 19:14, skrev Sara Golemon:
> On Wed, Oct 14, 2015 at 11:59 PM, Björn Larsson > <bjorn.x.larsson@telia.com> wrote: >> Given the reason against this RFC in the thread it would be interesting >> to know why HHVM decided to implement it? >> > Happy to answer, but I need to state a couple things first: > * I don't really care if this change lands. I'd kinda like it, but > it's not solving a massive problem for me. > * There aren't any compelling reasons against this. The only reason > given of any note that I've seen is: "There are no compelling reasons > in favor of it." And I'll agree with that. Like I just said, it's > not solving any major problems, and it's not going to cause any major > problems. It's just a tiny, vanishingly insignificant piece of > syntactic sugar which disappears before we even get to the AST. > > So again, could scarcely care less, so don't expect me to champion > either side, but you asked "why", so here it is: It makes code reviews > marginally less ugly. > > That's it. It's a tiny problem to solve, and likely saves less than > 100ms during diff reviews, but it's a solution to a problem. > > Yes, it's a problem which countless developers live with to no > significant negative consequence. Solo developers and small shops > won't care about this since they tend to not bother with code reviews. > FB has enough engineers working on its very large codebase though, > that nobody has it all paged in, so code reviews are mandatory, and if > we can spend five minutes of effort to loosen the parser rules in > exchange for saving 1/10th of a second on every diff review that > extends/shrinks a function call/signature with no overhead, then of > course we would. That's a fair exchange. > > Apologies if you were hoping for a compelling reason. > > -Sara >
No, I think this is a good answer and part of the motivation you state could in my eyes land in the RFC. I mean there are big organizations using PHP ;-) The feature itself is free to use and misusing it shouldn't be a reason against it. //Björn

Ryan Pallas

10 years ago
On Thu, Oct 15, 2015 at 11:14 AM, Sara Golemon <pollita@php.net> wrote:
> On Wed, Oct 14, 2015 at 11:59 PM, Björn Larsson > <bjorn.x.larsson@telia.com> wrote: > > Given the reason against this RFC in the thread it would be interesting > > to know why HHVM decided to implement it? > > > > Happy to answer, but I need to state a couple things first: > * I don't really care if this change lands. I'd kinda like it, but > it's not solving a massive problem for me. > * There aren't any compelling reasons against this. The only reason > given of any note that I've seen is: "There are no compelling reasons > in favor of it." And I'll agree with that. Like I just said, it's > not solving any major problems, and it's not going to cause any major > problems. It's just a tiny, vanishingly insignificant piece of > syntactic sugar which disappears before we even get to the AST. > > So again, could scarcely care less, so don't expect me to champion > either side, but you asked "why", so here it is: It makes code reviews > marginally less ugly. > > That's it. It's a tiny problem to solve, and likely saves less than > 100ms during diff reviews, but it's a solution to a problem. > > Yes, it's a problem which countless developers live with to no > significant negative consequence. Solo developers and small shops > won't care about this since they tend to not bother with code reviews. > FB has enough engineers working on its very large codebase though, > that nobody has it all paged in, so code reviews are mandatory, and if > we can spend five minutes of effort to loosen the parser rules in > exchange for saving 1/10th of a second on every diff review that > extends/shrinks a function call/signature with no overhead, then of > course we would. That's a fair exchange. > > Apologies if you were hoping for a compelling reason. > > But its an entirely stylistic choice to use trailing commas for cleaner
diffs. You could also use leading commas as well. If you made that a coding standard for the organization, you would not have needed to implement trailing am I right? function foo( $bar ,$baz ,$boo ) { ... } too_many_args( $this->id ,'some constant string' , 123 ); Wouldn't this give the same benefit as trailing commas when it comes to adding removing arguments - a single line diff?

Pedro Cordeiro

10 years ago
> Wouldn't this give the same benefit as trailing commas when it comes to > adding removing arguments - a single line diff?
It would. However, I see some merit in someone wanting trailing commas for diffs. Leading commas would break away from PSRs and one should not have to choose between a standard or clean reading. If someone chooses to lose their precious time implementing this, it would actually give some people a new feature (clean diffs while keeping PSR2 compliance) without taking anything away from people who oppose this. This is not objectively bad, it just adds an option to the pool, which I think is good. I see no reason to allocate resources to make this happen, but if someone does allocate their personal time into coding this, I wouldn't oppose a merge. But again, I have no vote. 2015-10-15 16:16 GMT-03:00 Ryan Pallas <derokorian@gmail.com>:

Sara Golemon

10 years ago
On Thu, Oct 15, 2015 at 12:32 PM, Pedro Cordeiro <pedronaroga@gmail.com> wrote:
> I see no reason to allocate resources to make this happen, but if someone > does allocate their personal time into coding this, I wouldn't oppose a > merge. >
There's a diff attached to the RFC. The actual implementation is two lines. The rest is a bunch of unit tests for positive and negative results. We've spent FAR more time discussing it than implementing it took. -Sara

Ryan Pallas

10 years ago
On Thu, Oct 15, 2015 at 1:32 PM, Pedro Cordeiro <pedronaroga@gmail.com> wrote:
> > Wouldn't this give the same benefit as trailing commas when it comes to > > adding removing arguments - a single line diff? > > It would. > > However, I see some merit in someone wanting trailing commas for diffs. > Leading commas would break away from PSRs and one should not have to choose > between a standard or clean reading. > > If someone chooses to lose their precious time implementing this, it would > actually give some people a new feature (clean diffs while keeping PSR2 > compliance) without taking anything away from people who oppose this. This > is not objectively bad, it just adds an option to the pool, which I think > is good. > > I see no reason to allocate resources to make this happen, but if someone > does allocate their personal time into coding this, I wouldn't oppose a > merge. > > But again, I have no vote. > > I want to clarify, I have no objection to this - was just asking if making
the decision that way back then at FB would have merited the same result. I'm in your boat, whether its added or not does not matter to me.

Sara Golemon

10 years ago
On Thu, Oct 15, 2015 at 12:16 PM, Ryan Pallas <derokorian@gmail.com> wrote:
> But its an entirely stylistic choice to use trailing commas for cleaner > diffs. You could also use leading commas as well. If you made that a coding > standard for the organization, you would not have needed to implement > trailing am I right? > > Wouldn't this give the same benefit as trailing commas when it comes to > adding removing arguments - a single line diff? >
IMO, that would solve the one small problem in exchange for a new small problem. That of the cognitive overhead of parsing leading commas where standard practice calls for trailing commas. And before you counter than the final optional comma also comes with overhead, let me point out that the array consistency argument. We have trailing commas there already, so no new mental overhead. -Sara

Ryan Pallas

10 years ago
On Thu, Oct 15, 2015 at 1:46 PM, Sara Golemon <pollita@php.net> wrote:
> On Thu, Oct 15, 2015 at 12:16 PM, Ryan Pallas <derokorian@gmail.com> > wrote: > > But its an entirely stylistic choice to use trailing commas for cleaner > > diffs. You could also use leading commas as well. If you made that a > coding > > standard for the organization, you would not have needed to implement > > trailing am I right? > > > > Wouldn't this give the same benefit as trailing commas when it comes to > > adding removing arguments - a single line diff? > > > IMO, that would solve the one small problem in exchange for a new > small problem. That of the cognitive overhead of parsing leading > commas where standard practice calls for trailing commas. > > And before you counter than the final optional comma also comes with > overhead, let me point out that the array consistency argument. We > have trailing commas there already, so no new mental overhead. > > Wasn't planning on countering, just trying to show that Hack made a
conscious choice to go this way. I personally don't see a benefit, as its not hard to read a diff that has a new comma on a previous line following by a new var/value on the next - but if people do then by all means add it in :) I do h ave one question I just thought of though... how does the allowance of trailing comma work with the splat operator (...)? I'm assuming a function call/definition may only have one or the other, is that correct?

Sara Golemon

10 years ago
On Thu, Oct 15, 2015 at 12:51 PM, Ryan Pallas <derokorian@gmail.com> wrote:
> I do h ave one question I just thought of though... how does the allowance > of trailing comma work with the splat operator (...)? I'm assuming a > function call/definition may only have one or the other, is that correct? >
Excellent question. The patch on Sammy's RFC was written in Feb 2013, before variadics, so it doesn't take them into account. I would say that it should be modified to disallow a trailing comma following either a variadic declaration or a splat invocation, since the grammar around these two things prohibits followups anyway. -Sara

Nikita Popov

10 years ago
On Thu, Oct 15, 2015 at 10:04 PM, Sara Golemon <pollita@php.net> wrote:
> On Thu, Oct 15, 2015 at 12:51 PM, Ryan Pallas <derokorian@gmail.com> > wrote: > > I do h ave one question I just thought of though... how does the > allowance > > of trailing comma work with the splat operator (...)? I'm assuming a > > function call/definition may only have one or the other, is that correct? > > > Excellent question. The patch on Sammy's RFC was written in Feb 2013, > before variadics, so it doesn't take them into account. > > I would say that it should be modified to disallow a trailing comma > following either a variadic declaration or a splat invocation, since > the grammar around these two things prohibits followups anyway. >
We allow multiple splats in one call. Something like this (after trailing comma patch) should be just fine: foo( ...$args, ...$moreArgs, ...$evenMoreArgs, ); We don't allow multiple variadic parameters. However I also see little point in explicitly forbidding this one case. You'd be able to use the trailing comma in all cases ... apart from that one single instance. Nikita

Sara Golemon

10 years ago
On Thu, Oct 15, 2015 at 1:08 PM, Nikita Popov <nikita.ppv@gmail.com> wrote:
>> I would say that it should be modified to disallow a trailing comma >> following either a variadic declaration or a splat invocation, since >> the grammar around these two things prohibits followups anyway. > > > We allow multiple splats in one call. Something like this (after trailing > comma patch) should be just fine: > > foo( > ...$args, > ...$moreArgs, > ...$evenMoreArgs, > ); > > We don't allow multiple variadic parameters. However I also see little point > in explicitly forbidding this one case. You'd be able to use the trailing > comma in all cases ... apart from that one single instance. >
Derp, right. Sorry. I don't really know PHP. Is it like Ruby? -Sara

Marcio Almada

10 years ago
Hi, 2015-10-14 16:25 GMT-03:00 Sammy Kaye Powers <me@sammyk.me>:
> Hello internals friends! > > I'd like to open a discussion on the RFC to allow trailing commas in > function arguments. > > https://wiki.php.net/rfc/revisit-trailing-comma-function-args > > Discuss! :) > > Thanks, > Sammy Kaye Powers > sammyk.me
Sammy, I think this proposal is too narrow. If we are going to bother to change the language, then let's do it consistently and allow trailing commas on all lists in the grammar. These are the ones on mind right now: - use declarations - group use declarations - function call argument list - class member lists (both constants and properties) - argument list declarations - arrays (already allowed) As exemplified in this gist: https://gist.github.com/marcioAlmada/75f8f1d47da5dcac2e57 Why? Because it prevents a reality where each PHP minor version introduces trailing commas in a new different place, hurting code portability. Because it couldn't get more consistent. And more importantly: because we would get rid of this discussion about trailing commas, forever j/k :) ty, Márcio

Sammy Kaye Powers

10 years ago
On Thu, Oct 15, 2015 at 8:28 PM, Marcio Almada <marcio.web2@gmail.com> wrote:
> Hi, > > 2015-10-14 16:25 GMT-03:00 Sammy Kaye Powers <me@sammyk.me>: > > Hello internals friends! > > > > I'd like to open a discussion on the RFC to allow trailing commas in > > function arguments. > > > > https://wiki.php.net/rfc/revisit-trailing-comma-function-args > > > > Discuss! :) > > > > Thanks, > > Sammy Kaye Powers > > sammyk.me > > Sammy, I think this proposal is too narrow. If we are going to bother > to change the language, then let's do it consistently and allow > trailing commas on all lists in the grammar. These are the ones on > mind right now: > > - use declarations > - group use declarations > - function call argument list > - class member lists (both constants and properties) > - argument list declarations > - arrays (already allowed) > > As exemplified in this gist: > https://gist.github.com/marcioAlmada/75f8f1d47da5dcac2e57 > > Why? Because it prevents a reality where each PHP minor version > introduces trailing commas in a new different place, hurting code > portability. Because it couldn't get more consistent. And more > importantly: because we would get rid of this discussion about > trailing commas, forever j/k :) > > ty, > Márcio >
Hey Márcio! Thanks for the suggesting "trailing comma all the things" idea. I can certainly get behind that idea and I think it would offer an even stronger argument than just trailing commas on function args. I'm curious what others think about broadening the scope of the RFC to include all the list grammars in PHP.

Björn Larsson

10 years ago
Den 2015-10-16 kl. 15:09, skrev Sammy Kaye Powers:
> On Thu, Oct 15, 2015 at 8:28 PM, Marcio Almada <marcio.web2@gmail.com> > wrote: > >> Hi, >> >> 2015-10-14 16:25 GMT-03:00 Sammy Kaye Powers <me@sammyk.me>: >>> Hello internals friends! >>> >>> I'd like to open a discussion on the RFC to allow trailing commas in >>> function arguments. >>> >>> https://wiki.php.net/rfc/revisit-trailing-comma-function-args >>> >>> Discuss! :) >>> >>> Thanks, >>> Sammy Kaye Powers >>> sammyk.me >> Sammy, I think this proposal is too narrow. If we are going to bother >> to change the language, then let's do it consistently and allow >> trailing commas on all lists in the grammar. These are the ones on >> mind right now: >> >> - use declarations >> - group use declarations >> - function call argument list >> - class member lists (both constants and properties) >> - argument list declarations >> - arrays (already allowed) >> >> As exemplified in this gist: >> https://gist.github.com/marcioAlmada/75f8f1d47da5dcac2e57 >> >> Why? Because it prevents a reality where each PHP minor version >> introduces trailing commas in a new different place, hurting code >> portability. Because it couldn't get more consistent. And more >> importantly: because we would get rid of this discussion about >> trailing commas, forever j/k :) >> >> ty, >> Márcio >> > Hey Márcio! > > Thanks for the suggesting "trailing comma all the things" idea. I can > certainly get behind that idea and I think it would offer an even stronger > argument than just trailing commas on function args. > > I'm curious what others think about broadening the scope of the RFC to > include all the list grammars in PHP. >
Well, if there is a wish to broaden the scope maybe the voting should be split on each case? Like the remove depr. func. RFC: https://wiki.php.net/rfc/remove_deprecated_functionality_in_php7 r//Björn

Albert Casademont Filella

9 years ago
Revisiting this thread. AFAIK this RFC was never put to vote, right? On Fri, Oct 16, 2015 at 9:01 PM, Björn Larsson <bjorn.x.larsson@telia.com> wrote:

Dan Ackroyd

9 years ago
On 1 September 2017 at 13:16, Albert Casademont <albertcasademont@gmail.com> wrote:
> Revisiting this thread. AFAIK this RFC was never put to vote, right? >
It was covered in https://wiki.php.net/rfc/list-syntax-trailing-commas which was put to a vote. Although I would like see just the trailing commas in function arguments be brought up for discussion again, without any of the other distracting stuff - like trailing commas in function declarations - it probably would be better to wait, to avoid people being annoyed by the same RFC being brought up too soon. cheers Dan

Björn Larsson

9 years ago
Hi, Recall that it was superseded by this RFC, partly implemented in PHP 7.2: - https://wiki.php.net/rfc/list-syntax-trailing-commas In my eyes the voting result created a not quite consistent view on how trailing commas are handled in PHP in general. r//Björn Larsson Den 2017-09-01 kl. 14:16, skrev Albert Casademont: