Add support for trailing commas in Emitter

[srawlins]

Fixes dart-lang/tools#990

This adds trailing commas in various places according to the following rules:

• in a collection literal if there are more than one element
• in an argument list if there are more than one argument (positional or named)
• in a constructor parameter list, method parameter list, or function type parameter list, if there are more than one parameter (positional, optional, or named)

The last one was certainly the most complex, because of the optional/named delimiters ([]/{}).

• A parameter list with one positional or one optional or one named parameter does not get a trailing comma.
• A parameter list with multiple positional parameters and no optional or named parameters gets a trailing comma.
• A parameter list with at least one optional parameter or at least one named parameter, and at least two parameters in total, gets a trailing comma before the closing delimiter (] or }).

[natebosch]

I'm nervous about sending the signal that code_builder is intended to make "pretty" output, although nothing immediately comes to mind where we might need to have ugly output.

I would prefer if we err on the side of being opinionated over being configurable. If we think trailing commas is a better default I'd like to use it everywhere, rather than increase the API surface. WDYT @jakemac53 ?

[natebosch]

I guess one other area that may come up is that we explicitly don't want to add options to have output that conforms to style lints. I would not accept PRs that, for instance, have options to conform to prefer single or double quotes, or to sort members within a class in a certain order.

[srawlins]

I would prefer if we err on the side of being opinionated over being configurable.

I don't mind this idea. I thought there might be a question of breakage (internal goldens?) but if code_builder has some contract that we can change the textual content of the output code, then that's great.

I guess one other area that may come up is that we explicitly don't want to add options to have output that conforms to style lints.

Yeah my motivation here is 100% for the performance concerns in dart_style. I took the effort to make a distinction between 1-element lists and multiple-element lists to make the generated output bearable, for developers writing builders, and users ctrl-clicking into generated code. But if it comes up against a principle, I'm find just adding commas anywhere.

[sigurdm]

Maybe it would be worth considering some recursive logic. If the single argument of an invocation itself uses trailing comma, then it should probably do as well...

Compare:

    foo(bar(
      takes,
      a,
      lot,
      of,
      arguments,
    ));

to

    foo(
      bar(
        takes,
        a,
        lot,
        of,
        arguments,
      ),
    );

I much prefer the second...

[jakemac53]

I am fine with just always using them, at least for argument lists and list/map/set literals?

The place I really dislike the style is for parameter lists.

[srawlins]

I've updated the code to always consider adding trailing commas. Trailing commas are still only added if there are multiple elements in a list. I deleted the tests I added as I think everything is covered with existing tests.

This feature still applies to parameter lists; let me know if it should not.

[jakemac53]

This feature still applies to parameter lists; let me know if it should not.

I think it's fine, I will survive lol. Technically parameter lists could also get quite large, and bob also mentioned that if you have arrow body functions with a large expression, trailing commas in parameter lists will help.

[natebosch]

Can you add a CHANGELOG?

[natebosch]

I thought there might be a question of breakage (internal goldens?) but if code_builder has some contract that we can change the textual content of the output code, then that's great.

Yeah we have made changes to the specific output in the past - we consider a test on the specific content output to be opting in to needing updates.

It will make the next package roll more annoying since we will have to atomically update any golden tests, but that's typically pretty manageable.

[srawlins]

Changelog added; thanks!