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:

• Preserve trailing commas #1652
• Should argument lists with named arguments always split? #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: munificent/temp_arg_split_sample@bafb62f

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.