I do really like the change, as it is damn complicated to extend an array if it fails all the time as the previous line did not had a comma.

you should also allow them in arrays (for which your consistency argument would be even more accurate as PHP does not support it at the end of function call arguments)

Arrays already good, I tested them.

Trailing commas in arrays are supported by \Doctrine\Common\Annotations\DocParser::Arrayx() and I manually confirmed that as well.

Also, anything that decreases the chances of WTFs is an improvement.

Working with this daily in Drupal, and also dealing with overcoming this particular objections with Drupal devs very very regularly. I 100% support this.

Whenever I'm writing JSON and forget that one comma there - I'm wasting another minute of my lifetime to get rid of it.

Let's make writing Annotations less painful and get this in, please!

I think there's a better way to support this without entering on 2nd level of indentation.
Will look into it. =)

I am not sure what you mean -- I could look at the lookahead token retrieved instead of calling the helper but calling the helper seemed cleaner. If you'd clarify what you'd like me to do, I'd be glad to help more.

@chx the issue I see is that you're mixing grammar rules by checking a token that is not part of "Values", but its parent, "Annotation".
There may be other ways to achieve same solution by rewriting the grammar rule, for example and updating the code to match it. Quick and dirty example would be from Value {"," Value}* to {Value ","}* Value or even {Value ","}+. Each of these have its own problem, but none of these covers the unknown token ).
I'm considering a variance of {Value ","?}+, but this one would make this annotation valid:

/**
 * @Annotation(foo="bar" bar="woo", fur="buzz")
 */

That's why I'm telling that it may be a little bit different your patch.

I simply upgraded the grammar to be Values ::= Array | Value {"," Value}* ,? but that might be wrong. I am not quite sure, however, how to check we are finished if I can't look at the forthcoming parenthesis...

@chx that's why I told you there may be a better approach for the problem. I just can't think on this atm (deploying a subsystem to production at work)

I checked the aforementioned arrayx function and it actually does the exact same. Added in 3b9d2f8

I asked this on stackoverflow and fixed my grammar syntax.

Worked with Damien Tournoud on this; the grammar is LL(1) again. Tests passed.

Also note that the grammar is positively weird:

Values ::= "(" [Array | Value {"," Value}* [","] ] ")"
Value ::= PlainValue | FieldAssignment
PlainValue ::= integer | string | float | boolean | Array | Annotation

So this means that {}, "foo" is invalid but "foo", {} is valid (PlainValue can contain Array).

What about (in a followup) removing the special casing of Array from Values?

Is there any feedback on this in terms of what's left to get it committed?

Ran into this problem too. Kinda annoying. The syntax error that gets thrown in the least is uninformative, especially as commas are easily overlooked characters.

Does this issue need to be assigned to a specific person in order to be reviewed for acceptance?

@guilhermeblanco ping again

Fixed here: 463d926
I kept BC, but still rely on 1 misleading call of CLOSE_PARENTHESIS where it shouldn't... couldn't imagine a proper solution for now.