[flutter] (prefer) trailing_commas

[pq]

Following-up from flutter/flutter-intellij#2980 (comment), we might consider a rule that enforces the flutter style guide suggestion:

Use a trailing comma for arguments, parameters, and list items, but only if they each have their own line.

Since a major motivation is to give analyzer something to hang quick-fixes from, we might consider enforcing just the first part and allowing dartfmt to handle line-breaks but maybe this would make the rule less useful for flutter repo committers.

@Hixie: any thoughts?

/cc @jaumard

[srawlins]

FYI I have a request for dartfmt --fix to add support for this too: dart-lang/dart_style#753

[a14n]

I started a lint for this case and I think there should be at least 2 lints.

One to lint missing coma in the following cases (flutter code that don't use dartfmt)

void f(
  int p1,
  int p2
) {
  ....
}

An other one (really harder to do) that takes some list of params/args/elements and advices to add a trailing coma to make the formatted code (with ``dartfmt) looks better.

void f(int parameter1, int parameter2, int parameter3, int parameter4,
    int parameter5, int parameter6, int parameter7, int parameter8) {
  //
}

// after trailing coma

void f(
  int parameter1,
  int parameter2,
  int parameter3,
  int parameter4,
  int parameter5,
  int parameter6,
  int parameter7,
  int parameter8,
) {
  //
}

Another simpler lint could be to check trailing comas in widget trees.

[bwilkerson]

For the record, I'm not convinced that the value of a lint like this would justify the cost (cognitive load for users, maintenance cost for us). How many users would be helped by having this lint? (And for how long? Seems like after a couple of days most users would learn to add commas without needing a prompt.)

That said, writing the lint should be fairly trivial. For every list you want to check, get the last element, get it's end token, and look to see whether the next token is a comma. If you only want to check lists whose elements are already on separate lines (or whose first and last elements are on different lines, or whatever) then use the tokens to get offsets and use the LineInfo to map those offsets to line numbers and compare those.

[a14n]

That said, writing the lint should be fairly trivial. For every list you want to check, get the last element, get it's end token, and look to see whether the next token is a comma. If you only want to check lists whose elements are already on separate lines (or whose first and last elements are on different lines, or whatever) then use the tokens to get offsets and use the LineInfo to map those offsets to line numbers and compare those.

That's what I did :)

There are several problems.

In the flutter codebase (that don't use dartfmt) a missing trailing comma don't really have a impact until someone copy-paste code to put it in a place that will use dartfmt. Trees will suddently be flattened if a trailing comma is missing.

In a project that uses dartfmt the above rule to trigger the lint couldn't be used. As my second snippet shows, dartfmt formats f on two lines. No lint will be triggered here.

[Hixie]

Flutter would totally use this lint.

Seems like after a couple of days most users would learn to add commas without needing a prompt.

I've been writing code in this style for a couple of years and I still regularly omit the comma by mistake (then hate myself when I try to add another item to the list, and the compiler and analyzer lose their mind because they can't figure out what I did wrong).

[Hixie]

(Incidentally this is one case where using the formatter would make things ten times worse, because omitting the comma turns the entire list into a completely different style that is even less easy to add items to using copy and paste.)

[bwilkerson]

Flutter would totally use this lint.

Good to know, thanks.

In the flutter codebase (that don't use dartfmt) a missing trailing comma don't really have a impact until someone copy-paste code to put it in a place that will use dartfmt. Trees will suddently be flattened if a trailing comma is missing.

In a project that uses dartfmt the above rule to trigger the lint couldn't be used. As my second snippet shows, dartfmt formats f on two lines. No lint will be triggered here.

I think Flutter wants a rule that users should always include a comma at the end of parameter and argument lists. Is that true?

But I don't understand the non-flutter / dartfmt case. What is the rule you're proposing for that case?

[a14n]

I think Flutter wants a rule that users should always include a comma at the end of parameter and argument lists. Is that true?

(my feeling) 2 use-cases:

1. As a developer on Flutter codebase I don't use dartfmt so in trees of widget it's common to forget trailing commas. As example this code snippet is from flutter:

  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(
        backgroundColor: Theme.of(context).canvasColor,
        elevation: 0.0 // OOOOOOOOOOOOOOOOPS
      ),
      body: Column(
        crossAxisAlignment: CrossAxisAlignment.stretch,
        children: <Widget>[
          // Give the key-pad 3/5 of the vertical space and the display 2/5.
          Expanded(
            flex: 2,
            child: CalcDisplay(content: _expression.toString()) // OOOOOOOOOOOOOOOOPS
          ),
          const Divider(height: 1.0),
          Expanded(
            flex: 3,
            child: KeyPad(calcState: this) // OOOOOOOOOOOOOOOOPS
          ),
        ] // OOOOOOOOOOOOOOOOPS
      ) // OOOOOOOOOOOOOOOOPS
    );
  }

As you may have notice some trailing commas are missing. So when someone add an argument or a element in a list he could face errors described above by @Hixie. An other issue is that if someone copy/paste this code in a project that use dartfmt the widget tree will be flattened to:

  Widget build(BuildContext context) {
    return Scaffold(
        appBar: AppBar(
            backgroundColor: Theme.of(context).canvasColor, elevation: 0.0),
        body: Column(
            crossAxisAlignment: CrossAxisAlignment.stretch,
            children: <Widget>[
              // Give the key-pad 3/5 of the vertical space and the display 2/5.
              Expanded(
                  flex: 2, child: CalcDisplay(content: _expression.toString())),
              const Divider(height: 1.0),
              Expanded(flex: 3, child: KeyPad(calcState: this))
            ]));
  }

Here a good lint (for developers on flutter) would be to require a trailling comma if the closing parenthesis or bracket starts the line. (that's what I do in a lint I have locally - the 6224 fixes can be seen in a14n/flutter@08f7785)

2. As a Flutter user that uses dartfmt on my project, I'd like to have a lint to make the above formatted code in something that looks better (like a tree). Here we could not have the same logic as for the lint above. Perhaps we could check if an Object created is a Widget and require its arguments to have a trailing commas. We could also add a @tree (or something like that) on classes (like Widget) to drive the lint rule when to check for trailing commas.

[bwilkerson]

Is this a fair summary?

• In hand-formatted (Flutter) code you're using the formatting as a signal as to when a comma was intended but forgotten.

• In auto-formatted code we're not sure what the right signal is (because the intent has been lost).

It would be great to find a shared signal so that we don't need two rules, so let me ask a couple of questions.

Why are some widgets (such as AppBar and Expanded) not formatted on multiple lines? (My guess, having not looked at a lot of Flutter code) is that the widgets with children are formatted differently than widgets without children.)

Does the style of formatting arguments on multiple lines apply to anything other than widgets?

[Hixie]

I think Flutter wants a rule that users should always include a comma at the end of parameter and argument lists. Is that true?

Here are some examples where you wouldn't want a trailing comma:

  void foo(String bar) {
  }

  var bar = [1, 2, 3];

  quux(bar, baz);

Here are some where you would:

  void foo(
    String bar,
  ) {
  }

  var bar = [
    1,
    2,
    3
  ];

  quux(
    bar,
    baz
  );

[a14n]

Why are some widgets (such as AppBar and Expanded) not formatted on multiple lines?

My above sample (without commas for AppBar and Expanded) was code from a sample that should have used trailing commas. There are just typo that need to be fixed. So AppBar and Expanded are not exceptions.

Regarding the samples @Hixie put in its last comment. It's what I described as the logic for the first rule and what I already have implemented locally.

The second case is not only specific to Flutter. If you look at dart-lang/dart_style#753 it's related to angular. That's why I mentioned a hypothetical @tree.