Split argument lists that would otherwise fit

[munificent]

The most consistent negative feedback we've gotten from the new formatting style is that it often puts an argument list on one line because it fits the page width when the user feels the resulting code would look better if split. For example (based on #1652):

// Current behavior:
Material(
  color: Colors.transparent,
  child: InkWell(
    customBorder: const CircleBorder(),
    child: Padding(padding: const EdgeInsets.all(2), child: child),
    onTap: () {
      context.read<ScGalleryVm>().crop();
    },
  ),
)

// Preferred:
Material(
  color: Colors.transparent,
  child: InkWell(
    customBorder: const CircleBorder(),
    child: Padding(
      padding: const EdgeInsets.all(2),
      child: child,
    ),
    onTap: () {
      context.read<ScGalleryVm>().crop();
    },
  ),
)

Even though the Padding(...) call fits on one line, many users feel the latter code is easier to read and maintain.

The new formatter's configurable page width exacerbates this issue. If you set a wider page width, you'll get even more of these nested long argument list lines.

I'm filing this issue to be the sort of "meta-issue" that discusses this problem. If you have examples of argument lists where you think the formatter should have split them and didn't, please include them here (as text, not screenshots!). Then we can discuss proposed solutions to this problem as separate issues:

• #1652
• #1647

[yakagami]

Another related example: Lists of widgets (Row, Column, ListView) read better when each item in the List has its own line.

Current:

Widget example() {
  return const Row(children: [Text("1"), Text("2"), Text("3")]);
}

Desired:

Widget example() {
    return const Row(
        children: [
            Text("1"),
            Text("2"),
            Text("3"),
        ]
    );
}

Not sure if this is technically an argument list though, because there is only one argument (the List literal).

[munificent]

OK, I've taken a random sample of Dart code on pub and prototyped a few tweaks to argument splitting. The repo is here. The main branch is formatted using the current tall style formatter. Each branch applies a different tweak and then commits the changes. The commits for each branch are:

• any-named-arg: Splits any argument list containing a named argument.
• multiple-named-and-call: Split any argument list with multiple named arguments and at least one function call argument.
• ctor-context: Split argument lists and list literals that look like they are inside constructor call trees.

My take is that the first rule is too aggressive, the second is too conservative, and the third does pretty good though the rules are fairly subtle. I'd like to hear what others think. Take a look at the diffs. Is there code you think should split that didn't? Is there code that didn't split that you think should?

At a high level, I think the goal we're all generally aiming for is that in "markup-like code", calls that "create objects" should generally each be on their own line. Of course, the scare quotes there indicate that this isn't trivial since the formatter doesn't really know what's markup-like code versus imperative, and it doesn't know which expressions are creating objects.

But I think assuming that a capitalized function name is a constructor works pretty well in practice. The formatter has already long used that heuristic when formatting method chains and no one seems to have noticed or minded.

[knopp]

The more I look at the code the more I'm convinced that taking control away from users is a bad idea. Whether I want split the code into multiple lines doesn't just depend on structure of the code, it depends on what kind of code is it and in what context. For widgets we tend to add trailing coma quite aggressively, but I'd for example not do this (third example):

[nex3]

Personally I'd be satisfied with the ctor-context heuristic. I don't consider logical subtlety to be a huge downside in a formatter; the point is for humans not to think about formatting, and the difference between not thinking about a simple heuristic and not thinking about a complex heuristic is pretty negligible. If the complex heuristic produces better output, I'm happy.

One thing that might also be worth pointing out is that, as far as I've seen from the examples posted, Flutter-style declarative widget constructors tend to have only named arguments. It may be worth incorporating that into the heuristic, possibly by having the checks for list/constructor-like arguments only apply to named arguments. For example, I'd prefer SassList([...initial, before], separator: ListSeparator.space) not to split.

[munificent]

Flutter-style declarative widget constructors tend to have only named arguments.

That's mostly true. Text() is the main exception. But this is a good observation. I went ahead and tried that out on the sample. Here's the diff versus the ctor-context branch: https://github.com/munificent/temp_arg_split_sample/commit/bafb62fd37b631156c22c131d1e2e83460969d48

A handful of argument lists no longer split. To my eye, those all look like improvements. @knopp, this also addresses the Get.put() example you mentioned.

[natebosch]

I think the examples that have been the most compelling to me are the ones where the non-broken constructor call is nested inside a call that is split.

This example looks totally fine to me and I worry about the impact on non-flutter code if we force it to split.

Widget example() {
  return const Row(children: [Text("1"), Text("2"), Text("3")]);
}

I am swayed by the Padding in this example, because it breaks the pattern around it.

// Current behavior:
Material(
  color: Colors.transparent,
  child: InkWell(
    customBorder: const CircleBorder(),
    child: Padding(padding: const EdgeInsets.all(2), child: child),
    onTap: () {
      context.read<ScGalleryVm>().crop();
    },
  ),
)

I think it would be interesting to explore a variant is like any-named-arg but where the forced splitting only takes effect one "layer" down. I think that might resolve the concerns for SassList (at least I don't recall seeing it often used as an expression passed to a named argument) and I'm fairly confident it would resolve most of my concerns for the impact in non-flutter code.

The heuristic I was thinking about when reading through examples is roughly: Always split calls with named arguments when they are in an expression passed to a named argument.

So:

  Column(children: [Text('text1'), Text('text2')]);

  Padding(
    padding: padding,
    child: Column(
      children: [Text('text1'), Text('text2')],
    )
  );

  Padding(
    padding: padding,
    child: Column(
      children: [
        Text(
          'text1'
          style: textStyle,
        ),
        Text('text2'),
      ],
    )
  );

It's hard to tell how much of a problem it would be to not split at the "top level" - there are examples cited on these threads but at least some strike me as simplified pieces of larger trees.

In my opinion it doesn't make sense to treat any collection like it was a call with a named argument, but that might be a design choice where I'm a bit further from the consensus.

[nex3]

That's mostly true. Text() is the main exception. But this is a good observation. I went ahead and tried that out on the sample. Here's the diff versus the ctor-context branch: munificent/temp_arg_split_sample@bafb62f

I think this may be implementing something slightly different than what I meant to suggest. The first example in that diff changes

  Center(
    child: Text(
      'data',
      style: TextStyle(fontSize: 16, color: Colors.blue),
    ),
  );

into

  Center(
    child: Text('data', style: TextStyle(fontSize: 16, color: Colors.blue)),
  );

whereas in my proposal, that Text() call would still split because there's a constructor-like call TextStyle(...) in a named argument (style:).

@natebosch's heuristic sounds pretty good to me as well, although I'd definitely be curious to see how it played out in real code.

[HideakiSago]

• any-named-arg: Splits any argument list containing a named argument.
• multiple-named-and-call: Split any argument list with multiple named arguments and at least one function call argument.
• ctor-context: Split argument lists and list literals that look like they are inside constructor call trees.

Thank you for providing the sample! Very easy to compare!

I think each has its pros and cons.

any-named-arg: Splits any argument list containing a named argument.

Pros

• The rules are simple and very consistent, so it's easy to understand. (I think this is an important point to consider when automating code formatting)

Cons

• I didn't want a line break when there was only one argument, regardless of whether it was named or not.

multiple-named-and-call: Split any argument list with multiple named arguments and at least one function call argument.

The rules are complex and I don't fully understand them, but...

Pros

• It's easy to accept intuitively.
• It's relatively consistent and easy to understand.

Question

• I don't understand why some lines don't break.

List literal

If there are multiple elements in a List literal, wouldn't it be better to break the line?

Column(children: [Text('text1'), Text('text2')]);

I prefer this.

Column(
  children: [
    Text('text1'),
    Text('text2'),
  ],
);

TextStyle?

Why aren't arguments to TextStyle subject to line breaks?

Center(child: Text('data', style: TextStyle(fontSize: 16, color: Colors.blue)));

I prefer this.

Center(
  child: Text(
    'data',
    style:
      TextStyle(
        fontSize: 16,
        color: Colors.blue,
      ),
    ),
  );

ctor-context: Split argument lists and list literals that look like they are inside constructor call trees.

Pros

• The definition of "constructor-like call" seems shaky, but it's an interesting observation.

Cons

• This code style was the hardest for me to understand. https://github.com/munificent/temp_arg_split_sample/blob/b148638aaa8e70510b7d0ec9fa98635e8f265ae8/acl_sdk_app_config.dart#L54-L58

    return (await (update(appConfig)..where((item) => item.id.equals(1))).writeReturning(
      AppConfigCompanion(
        lastSync: Value(lastSync),
      ),
    )).firstOrNull;

Isn't it supposed to be like this?

    return (await (update(appConfig)..where((item) => item.id.equals(1)))
        .writeReturning(AppConfigCompanion(lastSync: Value(lastSync))))
        .firstOrNull;

Question

Is this not broken down into lines? https://github.com/munificent/temp_arg_split_sample/blob/b148638aaa8e70510b7d0ec9fa98635e8f265ae8/_issue_examples.dart#L69

      style: TextStyle(fontSize: 16, color: Colors.blue),

Additional comments

One thing to be careful of is that if you can avoid it, it's better to avoid large changes in structure just by changing a little bit of code. Generally, you'll compare diffs in commits, so it's better to be able to see at a glance what has been changed. So, I think it's best to keep rules as simple as possible, and applying exceptional cases is not very desirable.

[mateusfccp]

multiple-named-and-call is how I would format my code 90% of the time. There are a few differences, but I can live with those perfectly.

I vote infinitely on this one.

[Nyaacinth]

I see ctor-context's result satisfying inside Flutter DSL most of the time, but still it produces some surprising result when it's not exactly Flutter DSL. I will vote this if I have to accept automated removing of trailing commas since it's the most unsurprising result if that comes from hand-formatting.

[yakagami]

I like @natebosch 's heuristic for identifying declarative code (a constructor call with named arguments nested in a constructor with named arguments). But I don't understand why in

  Padding(
    padding: padding,
    child: Column(
      children: [Text('text1'), Text('text2')],
    )
  );

the list is not split. I think if we are two constructor calls with named arguments in, its fair to assume that we are in a declarative expression and we can split the list literal as well. I'm OK with not spiting lists with only one named constructor due to non-flutter code, although I personally would always split Column(children: [Text('text1'), Text('text2')]). I'd be interested to see the diffs on that.

[Wdestroier]

~~Apparently something all these examples have in common is they have 2 or more parameters.~~

Edit: @mateusfccp pointed multiple-named-and-call would fix the example below and apparently it's better than the suggestion above.

A weird example I wrote:

  @override
  Widget build(BuildContext context) {
    return SelectionArea(
      child: Scaffold(appBar: FsMenu(), body: Center(child: SizedBox.shrink())),
    );
  }

[ScottS2017]

(I tried writing this code with markdown but it's doing the same thing as the new formatter... forcing the code onto one line)

There are many times the new format ends up removing a comma and then putting multiple constructors on the same line:

Container(child: Text('String Here'))... // Comma removed by formatter

vs

Container( child: Text('String Here'), ), ...

The problem is that the nesting allows us to scan down the code and quickly see each constructor, on its own line. This can be read at a glance. Forcing multiple constructors onto a single line sometimes, and sometimes not, forces you to slow down quite a bit as you try to pay attention carefully so you don't miss anything.

We've trained ourselves to read this code at a glance with nesting for seven years. Please put it back.

[munificent]

OK, I have tried many many heuristics and rules and I think I've found a couple that work well. They mostly split Flutter UI code without being hardcoded to look for Flutter specifically. They should work well for any deeply nested markup-like code.

At the same time, they don't go overboard and split imperative code that users already seem to be happy with, like calls to expect() like:

    expect(await platform.getInstalledApps(appType: 'system'), []);

The rules are:

A call is a constructor call if it's a new or const expression or a method call with a capitalized name with an optional prefix and/or constructor name (Foo(), prefix.Foo(), Foo.named(), etc.).

2. A constructor call's argument list is eagerly split if either of:

   1. Contains multiple arguments and any constructor calls.

   2. Contains one named argument and any constructor calls.

3. A list, map, or set literal is eagerly split if either of:

   1. Contains any other non-empty list, map, or set literals. (This rule already existed.)

   2. Is itself an argument to a constructor call and contains multiple elements and at least one constructor call.

Using the case of the identifier to decide that a call is a "constructor" feels sketchy. But, for what it's worth, the formatter has had that logic for years. It uses it to decide how to split method chains that start with static method calls. It seems to do the right thing.

Of course, these heuristics aren't perfect and won't do as good a job as carefully choosing by hand which argument lists should and shouldn't split. But the goal is to do a good enough job that the handful of places where it doesn't get it right are rare enough that the benefit to not worrying about manual argument list splitting outweighs the readability cost of those argument lists.

Note that #1652 is still open as well. A better argument list splitting heuristic is not mutually exclusive with also supporting preserving trailing commas.

I've tried this out on a larger random corpus of code from pub. You can see the results here. Note that the diff obviously only shows places where it chose to split. To see the places where it chose to not split, you'll have to look at all the code.

Let me know what you think.

[knopp]

I spend much more time reading code than writing it. I wouldn't want to format code by hand, since that's a chore, but deciding where to put trailing comma doesn't feel like a chore to me, it feels like a worthwhile thing to do. So even if the heuristics gets it 90% correct, I still wound't be happy with the tradeoff.

This new iteration seems to be leaning much more towards tall code, but even in case where I personally wouldn't do it. i.e.

Navigator.pushReplacement(
   context,
   MaterialPageRoute(builder: (context) => Close()),
 );

Instead of

Navigator.pushReplacement(context, MaterialPageRoute(builder: (context) => Close()));

(edit) I'm not entirely certain on this one, but for latter ones the formatter is definitely too eager to split the lines for my taste.

Or

                  return attachThemeSwitcher
                       ? PlayxThemeSwitchingArea(
                           child:
                               child ??
                               Center(
                                 child: Text(PlayxTheme.index.toString()),
                               ),
                         )
                       : child ??
                             Center(
                               child: Text(PlayxTheme.index.toString()),
                             );

The top part looks particularly strange, given the original:

                  return attachThemeSwitcher
                       ? PlayxThemeSwitchingArea(
                           child: child ?? Center(child: Text(PlayxTheme.index.toString())),                         
                         )
                       : child ?? Center(child: Text(PlayxTheme.index.toString()));

Edit: Even the before is not ideal, but this

                               child ??
                               Center(
                                 child: Text(PlayxTheme.index.toString()),
                               ),

Definitely doesn't feel like something I'd leave there if I had control over the formatting.

[knopp]

Another one:

                 const SizedBox(height: 16),
                 Text(
                   state.message,
                   style: Theme.of(context).textTheme.titleMedium,
                 ),
                 const SizedBox(height: 16),

I personally would not split the Text widget. I don't think having theme on separate line is worth it But I can also appreciated that it's a preference. But I don't think the style is important enough to warrant line splitting since it doesn't affect widget structure.

original:

                 const SizedBox(height: 16),
                 Text(state.message, style: Theme.of(context).textTheme.titleMedium),
                 const SizedBox(height: 16),

But perhaps more importantly, non-widget code:

    final List<YahooFinanceCandleData> oldPricesList = [
       YahooFinanceCandleData(
         date: DateTime.parse('2021-09-29'),
         adjClose: 1,
       ),
       YahooFinanceCandleData(
         date: DateTime.parse('2021-10-01'),
         adjClose: 3,
       ),
       YahooFinanceCandleData(
         date: DateTime.parse('2021-10-04'),
         adjClose: 4,
       ),
     ];

vs

    final List<YahooFinanceCandleData> oldPricesList = [
       YahooFinanceCandleData(date: DateTime.parse('2021-09-29'), adjClose: 1),
       YahooFinanceCandleData(date: DateTime.parse('2021-10-01'), adjClose: 3),
       YahooFinanceCandleData(date: DateTime.parse('2021-10-04'), adjClose: 4),
     ];

I don't think splitting the data to extra lines is worth the space in this case.

[knopp]

Original:

            child: AppBar(
               bottom: TabBar(
                 tabs: [
                   Tab(icon: Icon(Icons.attach_money)),
                   Tab(icon: Icon(Icons.directions_car)),
                   Tab(icon: Icon(Icons.directions_transit)),               
                 ],
               ),
             ),

New:

            child: AppBar(
               bottom: TabBar(
                 tabs: [
                   Tab(
                     icon: Icon(Icons.attach_money),
                   ),
                   Tab(
                     icon: Icon(Icons.directions_car),
                   ),
                   Tab(
                     icon: Icon(Icons.directions_transit),
                   ),
                 ],
               ),
             ),

[yakagami]

The only one that looked weird to me was this:

//old
columns: columns.map((col) => DataColumn(label: Text(col))).toList(),
//new
columns:
   columns
       .map(
         (col) => DataColumn(
           label: Text(col),
         ),
       )
       .toList(),

Overall I think this is an improvement over the current formatter. I'm still open to looking at more options though. Except for the above case and the ternary issue spotted by @knopp, none of the changes seem outright wrong to me. I wonder what the diff for the Flutter repo looks like? (I guess, from before the format commit.) That repo is in my opinion one the best metrics.

[esDotDev]

OK, I have tried many many heuristics and rules and I think I've found a couple that work well. They mostly split Flutter UI code without being hardcoded to look for Flutter specifically. They should work well for any deeply nested markup-like code.

How does it perform with something like this?

Padding(padding: EdgeInsets.only(top: 2, left: 4), child: child);

We'd want

Padding(
  padding: EdgeInsets.only(top: 2, left: 4), 
  child: child
);

But it seems like it would produce something like this which is wasting far too much vertical space:

Padding(
   padding: EdgeInsets.only(
       top: 2, 
       left: 4,
    ),
   child: child,
)

[Wdestroier]

The formatting of the ternary operator is taking so much space.

[mateusfccp]

But it seems like it would produce something like this which is wasting far too much vertical space:

Padding(
   padding: EdgeInsets.only(
       top: 2, 
       left: 4,
    ),
   child: child,
)

This is exactly what I want.

[esDotDev]

But it seems like it would produce something like this which is wasting far too much vertical space:

Padding(
   padding: EdgeInsets.only(
       top: 2, 
       left: 4,
    ),
   child: child,
)

This is exactly what I want.

6 vertical lines to express a single set of padding values?? MADNESS!! :p

[Nyaacinth]

Now I'm just curious about the decision that adding and removing of trailing commas must exist or not at the same time without a configurable option. I know that they want to keep "reversibility," and only one style across the community. But most other formatters just didn't do the same and they have maintained various different configurations/styles for years or even decades. 😇

[mateusfccp]

6 vertical lines to express a single set of padding values?? MADNESS!! :p

See? This is nothing but subjective and personal preference. I think this is the reason many people are complaining about the new formatter, every person has a different opinion about what is more or less readable.

However, I think people shouldn't be so hard on this.

The heuristics I provided in #1647, if adapted to have a maximum of two named arguments before breaking instead of one, could generate the format you are expecting. Although it is not what I would personally do, I don't really mind having this formatting.

[HideakiSago]

I was able to accept the changes in this commit without feeling any discomfort.

(I haven't looked carefully at the unchanged parts, but are there any parts I should look at?)

If I had to pick one piece of code that caught my attention, it would be the following code.

         columns:
             columns
                 .map(
                   (col) => DataColumn(
                     label: Text(col),
                   ),
                 )
                 .toList(),

I understand that this is based on the rule that arguments to constructor calls should be on separate lines. (2. Contains one named argument and any constructor calls.)

I think it's mostly a matter of preference, but I think it would be better to keep this on one line.

In other words, I felt that the rule 2-2 should not exist.

         columns: columns.map((col) => DataColumn(label: Text(col))).toList(),

Or, a rule that splits "multiple arguments including named arguments" might be good. (like AND conditions)

Side effects and adapt of the new code style

Formatting existing code in a new style will have some side effects (as some have pointed out, some of them are redundant). However, I think these can be improved somewhat by changing the way the code is written. Rather than forcing the format style to match the way humans prefer to write, I think it would be more reasonable for humans to adapt to the new format style (if the change in writing style can avoid redundancy).

It seems that many people tend to be resistant to the increase in line breaks, and although I understand this sentiment, I accept these line breaks as an unavoidable side effect since we need to set a standard somewhere.

[HyperJames]

Fundamentally asking developers to ‘change’ the way they read and write dart code is very personal and 100% individual preference and adds unnecessary cognitive load (for something many see little to no benefit's of).

This needs to be 100% configurable as there is no ‘one’ single solution or single set heuristics which will apply to everyone / all use cases / all developers or all projects.

Not all, but a large portion of existing developers would prefer no change - which is what this is fighting against.

[HideakiSago]

I'll add a few things that I've noticed.

Doesn't this fall under the conditions of Constructor?

             Expanded(
               child: Slider(value: value, min: 0.1, max: max, onChanged: onChanged),
             ),

The reason it doesn't look like this is because the following conditions don't apply, right? (A call is a constructor call if it's a new or const expression or a method call with a capitalized name with an optional prefix and/or constructor name (Foo(), prefix.Foo(), Foo.named(), etc.).)

I thought the line breaks were more consistent.

             Expanded(
               child: Slider(
                 value: value,
                 min: 0.1,
                 max: max,
                 onChanged: onChanged,
               ),
             ),

Consistent line breaks and indents

I know there may be a certain number of people who dislike this, but I'd say that the below is more consistent and makes more sense.

     check(
       SetStyles(
         Style.bold,
         Style.foreground(Color4.red),
       ).toAnsiString(),
     ).equals('\x1B[1;31m');

     check(
       SetStyles(
         Style.bold,
         Style.foreground(Color4.red),
       )
         .toAnsiString(),
     )
       .equals('\x1B[1;31m');

However, I think this can be improved by changing the way it is written.

Improvements by writing style

I think this code will be criticized by many people, but

      final request = http.MultipartRequest(
         'POST',
         Uri.parse("$baseUri/v1/audio/translations"),
       );

I would improve it as follows. In accordance with the DRY principle.

      final request = http.MultipartRequest.post("$baseUri/v1/audio/translations");

[SoftWyer]

There's a "formatting discrepancy" with enums too. With simple enums that extend over the line length, a trailing comma is allowed:

enum Example {
  example1,
  example2,
  example3,
  example4,
  example5,
  example6,
  example7,
  example8,
  example9,
  example10,
}

With enhanced enums that include a semicolon, the trailing comma is removed.

enum Example {
  example1,
  example2,
  example3,
  example4,
  example5,
  example6,
  example7,
  example8,
  example9,
  example10;

  const Example();
}

Previously it was fine to leave the dangling semicolon on it's own line and reduce the diff noise if adding a new enum, e.g.

enum Example {
  example1,
  example2,
  example3,
  example4,
  example5,
  example6,
  example7,
  example8,
  example9,
  example10,
  ;

  const Example();
}

Appreciate this may not be to everyone's taste, and I can live without it, but thought I'd mention it.

[zambetpentru]

Is there a way to make the behaviour consistent in Compare Folders extension in Visual Studio Code? Where it keeps the , on the left side but removes it from the right file upon Save.

[adifyr]

Am I the only one that used to format my columns/rows like this:

@override
Widget build(BuildContext context) {
  return Column(crossAxisAlignment: CrossAxisAlignment.start, spacing: 2.0, children: [
    Text('Hello'),
    Text('World'),
  ]);
}

I would like to be able to do this again.

If you can, I would also like my builder functions to be able to do the following:

void showMyDialog() async {
  final result = await showDialog<bool>(builder: (ctx) => AlertDialog(
    title: Text('Pick a choice'),
    content: Text('Yes Or No?'),
    actions: [
      TextButton(child: Text('Yes'), onPressed: () => Navigator.pop(ctx, true)),
      TextButton(child: Text('No'), onPressed: () => Navigator.pop(ctx, false)),
    ],
  ));
  debugPrint('The result was: $result');
}