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
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/
I'll echo Andrea remarks,
-1, sorry.
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/
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
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:
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.meGiven the reason against this RFC in the thread it would be interesting
to know why HHVM decided to implement it?Regards //Björn
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
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
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?
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:
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?
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
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.
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
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?
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
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
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
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
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.meSammy, 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/75f8f1d47da5dcac2e57Why? 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.
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/75f8f1d47da5dcac2e57Why? 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árcioHey 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
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:
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.meSammy, 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/75f8f1d47da5dcac2e57Why? 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árcioHey 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_php7r//Björn
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
Hi,
Recall that it was superseded by this RFC, partly implemented in PHP 7.2:
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:
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 mailto:bjorn.x.larsson@telia.com> wrote: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 <mailto:marcio.web2@gmail.com>> wrote: Hi, 2015-10-14 16:25 GMT-03:00 Sammy Kaye Powers <me@sammyk.me <mailto: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 <https://wiki.php.net/rfc/revisit-trailing-comma-function-args> Discuss! :) Thanks, Sammy Kaye Powers sammyk.me <http://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 <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 <https://wiki.php.net/rfc/remove_deprecated_functionality_in_php7> r//Björn