Should argument lists with named arguments always split?

[munificent]

There's been discussion on #1253 and elsewhere that some users would prefer some argument lists to split even when they would fit within a single line. I'm open to the idea, but I don't know what heuristics would work best. A very simple one (at least as I understand it) suggested by @koji-1009 is: Always split argument lists with named arguments.

This has a couple of nice properties:

• My intuition is that users generally expect positional argument lists to be dense and single line when possible.
• Flutter code, which is almost more like "markup" than "code" seems to be where users want argument lists to split even when they don't have to. And those constructor calls tend to mostly use named arguments.

On the other hand, I'm sure there are function calls with named arguments that aren't inside Flutter build code and which would look pretty bad if forcibly split. Off the top of my head, I can't see anyone preferring:

Duration(
  minutes: 1,
);

Over:

Duration(minutes: 1);

There is a rule in the formatter for collection literals that says a collection literal will always split if it contains any nested ones. That gives you:

[
  [1],
]

Instead of:

[[1]]

Perhaps a similar rule for argument lists would help here. We could consider always splitting an argument list if it has named arguments and at least one of those arguments is also a function call. However note that that rule would not work for the actual example brought up by @koji-1009:

EdgeInsets.only(
  top: 10.0,
  right: 20.0,
)

(Personally, if this were my code, I would prefer that call to not be split if possible.)

[nex3]

I also would prefer that @koji-1009's example not be split, but I think perhaps a closer-to-optimal heuristic would be "always split argument lists with multiple arguments at least one of which is named".

[FMorschel]

Agreed with @nex3, focus on multiple. So for you example with Duration it would only split the line if you had more than one parameter being filled.

[munificent]

So for you example with Duration it would only split the line if you had more than one parameter being filled.

I personally would still be pretty sad if the formatter decided to split Duration(hours: 1, minutes: 2) as well.

[FMorschel]

We could consider always splitting an argument list if it has named arguments and at least one of those arguments is also a function call.

And maybe increase the number of parameters needed to split normally to three or more (if one is named)?

So:

EdgeInsets.only(top: 10.0, right: 20.0);
EdgeInsets.only(
  top: 10.0,
  right: 20.0,
  left: 20.0,
);
//
Duration(hours: 1, minutes: 2);
Duration(
  hours: 1, 
  minutes: 2,
  seconds: 30,
);

For Flutter code, I'm mostly thinking of Text here as a reference.

Text('My string');
Text('My string', style: style);
Text(
  'My string',
  style: Theme.of(context).textTheme.bodyMedium,  // Because `.of(` call
);
Text(
  'My string', 
  style: style,
  maxLines: 2,
);

And another case (I'm unsure if this is done today, so laying all out) I would also like for the formatter to consider is that if one of the named arguments is a closure with a block:

ValueListenableBuilder(
  valueListenable: valueNotifier,
  builder: (context, value, child) {
    return const Text('My test');
  },
);

This is not the best argument to this case, but I'd say that the above looks better than (consider smaller texts here):

MyBuilder(value: v, builder: (context) {
    return const Text('My test');
});

Only because it depends on the parameter names and variables you use to make that readable, so we'd need to define a better value for this.

[koji-1009]

Thank you for creating the issue!

I understand the sadness when Duration(hours: 1, minutes: 2) is split. However, this is probably because Duration is so simple that we can easily read it on a single line. For example, I think few people would be sad if the ThemeData argument was split.

For this reason, I commented that for "methods with at least one named argument" I wanted to keep the trailing comma. I prefer consistency, so I want the trailing comma to remain in more than one case. But from a readability point of view, I can understand if it is supposed to remain in two or more cases.

[nex3]

I understand the sadness when Duration(hours: 1, minutes: 2) is split. However, this is probably because Duration is so simple that we can easily read it on a single line.

I'm really confused why you don't also find this to be true for EdgeInsets. They have the same number of parameters and even the same parameter types.

[koji-1009]

Sorry if I have given you the wrong impression. I think EdgeInsets is as simple as Duration.

To me, Duration, EdgeInsets and ThemeData are all just objects that I want to keep trailing comma. I would not be sad to see the addition of trailing comma in neither Duration nor EdgeInsets. I can see why some people feel sad, that's all I'm saying.

[cabaucom376]

I have a wild idea that I’m not sure how feasible it is, and it would require a lot of work, but I want to think outside the box. What if the formatter applied the tall style to the widget tree’s build methods and/or return statements? I believe this would solve most of the cases where I find the new formatter to be a hindrance (see #1253 (comment)). Usually, I do a lot of code reorganization and parameter additions within the widget tree, and anything outside that I wouldn’t mind collapsing.

Edit: The more I think about this the less I'm convinced of this idea.

[mateusfccp]

Argument splitting (and trailing commas in the old style, which forced the argument splitting) is always very subjective, and even within a team working in the same project there's often disagreement in CR about what is more "readable".

The optimal approach would be to decide on reasonable heuristics that will make most people satisfied, but we won't ever please everyone (unless we make the formatter non-opinionated, which is a non-goal).

These are my personal heuristics to decide if I should split arguments or not:

• IF all the arguments are positional,
  • IF the code exceeds the page width, I split;
  • IF the code does not exceed the page width, I don't split;

// Doesn't exceed the page width
foo(bar, baz, qux, quux);

// Exceeds the page width
foo(
  veryLongBarArgument,
  veryLongBazArgument,
  veryLongQuxArgument,
  veryLongQuuxArgument,
);

// Even with a single argument, if it is too long,
// I split it.
foo(
  anArgumentSoLongThatByItselfItWillExceedPageWidth,
);

• IF all the arguments are named,
  • IF it has a single argument,
    • IF the code exceeds the page width, I split;
    • IF the code does not exceed the page width, I don't split;
  • IF it has multiple arguments, I split.

// Single short argument
foo(bar: bar);

// Single long argument
foo(
  bar: anArgumentSoLongThatByItselfItWillExceedPageWidth,
);

// Multiple arguments, even if they would fit the page width
foo(
  bar: bar,
  baz: baz,
  qux: qux,
);

• IF there are both positional and named arguments, use the "all named arguments" heuristic, which, in this case, would always split.

// Single long argument
foo(
  positionalBar,
  bar: anArgumentSoLongThatByItselfItWillExceedPageWidth,
);

// Multiple arguments, even if they would fit the page width
foo(
  bar,
  baz,
  qux: qux,
);

Using the old formatter, there was a few times when I broke these "rules" if I found out that breaking them would make it better, but this is an overview of what I usually did.

[munificent]

I have a wild idea that I’m not sure how feasible it is, and it would require a lot of work, but I want to think outside the box. What if the formatter applied the tall style to the widget tree’s build methods and/or return statements?

This is a totally reasonable idea and similar ideas have come up elsewhere over the years. The key technical problem is that the formatter doesn't know what code is a build method. The formatter works on completely unanalyzed ASTs. That makes it fast (this is why for years dart format --fix was much faster than dart fix), and it allows you to format code even while it happens to have type errors or other semantic errors.

That means that all of the formatting rules have to be syntactic in nature. The formatter knows what the code looks like, but it doesn't know what it means.

(Even if the formatter could analyze code, I still would prefer it to not rely on that when making formatting choices. It would be strange if the formatter did a great job with Flutter code, but a new framework comes along that looks and behaves just like Flutter but the formatter formats its code differently.)

[Clavum]

but a new framework comes along that looks and behaves just like Flutter but the formatter formats its code differently

It sounds like having the formatter analyze the AST is not an option, but nonetheless to add to the conversation, if it did, could this problem could be addressed by creating a new annotation (perhaps @tallFormat or @preferTall or such) that would be the indication of whether to prefer a trailing comma, rather than specifically targeting Flutter? To be clear, this annotation would be applied once near the definition of the argument list, not at each individual usage. For example, the Widget class itself could be annotated and then any constructor of any class extending Widget would be affected, and other frameworks could do the same.

Besides, it's possible for Widgets to be created anywhere, such as if a widget is defined in a static or global variable, so treating build methods special might not catch everything.

This would allow us to choose for each individual class what we think is best, a task that only humans can accomplish because it depends on the meaning of the class and it's arguments (Duration makes sense being horizontal because we're used to seeing hh:mm:ss horizontal, while in most other cases multiple named arguments should be split. Along these lines we might need a @preferShort annotation too). This would also continue to ensure consistency because the annotation is chosen by the maintainer of the class and all usages therefore follow the same pattern.

I understand this kind of defeats the intention of the new formatter to take away this kind of work from developers and being opinionated on what's best. However, I think most end-users could be discouraged from using the annotation so it would only be a task for package authors to consider, and we'd probably only use it in Flutter anyway.

Again, for those skimming through this discussion, Bob's comment above explains that non-syntactic solutions like this probably won't work, but this idea came across my mind recently and got me excited so I wanted to write it down :)

[HudsonAfonso]

Is it feasible to implement a line-wrapping mechanism that enforces line breaks when an invocation exceeds a configurable threshold of arguments?

[FMorschel]

One more thought:

I know this is a call-site issue, but I would actually prefer parameter lists to wrap when they contain any number of named arguments above one on constructors. I'm not sure, but I'd expect that to be true for most users since we usually have required and type before the actual name of the argument so this would definitely make reading parameter list definitions easier.

[cabaucom376]

@munificent would it be possible to have the analyzer scan parameters for constructors with named arguments?

// Short style for constructors with only unnamed parameters.
// All parameters are kept inline.
class Foo {
  const Foo(this.a, this.b, this.c);

  final int a;
  final int b;
  final int c;
}

// Short style for constructors with 2 or fewer named parameters.
// Parameters remain inline since there aren't many.
class Bar {
  const Bar({required this.a, required this.b});

  final int a;
  final int b;
}

// Tall style for constructors with 3 or more named parameters.
// Each parameter is placed on its own line.
class Baz {
  const Baz({
    this.a,
    this.b,
    this.c,
  });

  final int? a;
  final Foo? b;
  final Bar? c;
}

// Example of a constructor with default parameter values.
// This demonstrates how default values can be specified inline for named parameters,
// maintaining the short style formatting for constructors with two or fewer named parameters.
class Qux {
  const Qux({this.a = 1, this.b = 2});

  final int a;
  final int b;
}

// Example of a constructor with a default parameter being a constructor.
class Quux {
  const Quux({
    this.a = 1,
    this.b = const Foo(1, 2, 3)
  });

  final int a;
  final Foo b;
}

// Example of a constructor call formatted in tall style.
// Even if only one named parameter is provided, nesting a constructor call 
// (here, Foo) may encourage the use of tall style for clarity.
Baz _buildLongBaz() {
  return Baz(
    b: Foo(1, 2, 3),
  );
}

// Example of a constructor call formatted in short style.
// When the constructor call is simple and does not involve nested constructor calls
// with named parameters, short style formatting is used.
Baz _buildShortBaz() {
  return Baz(a: 100);
}

[Nyaacinth]

This (#1647 (comment)) seems like a possible solution. If the scanning of nested constructions can be implemented and paired with those constraints (argument numbers and types), that might be optimal.

[wliumelb]

These examples do not really matter. People may prefer one style over another, but it's not why so many people are whining about the recent change.

This is what people actually hate:

Container(padding: EdgeInsets.all(8), child: Text('Some text'))

Flutter developers make sense of the structure of the widget tree by looking for the 'child' word.

We have got used to spotting it at the beginning of a line.

Hidding it in the middle of a line just goes against our intuition and makes our brains hurt.

[Nyaacinth]

Reference: #1647 (comment)

This kind of clear splitting of children and parent widgets/components/whatever stays true in other frameworks (and their formatters) as well (you might argue... okay, it doesn't mean to follow other languages), like JB's Compose DSL, React-likes' JSX, lit's lit-html, and many more. There are reasons for this situation.

[cabaucom376]

@wliumelb, does my proposal not address this scenario? In theory, it should position the “tall style” on widget trees since they are usually composed of “child” or “children” properties that contain constructors themselves.

[mernen]

@cabaucom376 I don't think the formatter can know what is a constructor call. We can assume that a capitalized name (Foo()) is a constructor, but that fails with named constructors (and also static methods that effectively act as constructors). Still, I think it's a good basis. I'd like to propose this:

• If a function or constructor call contains at least two named parameters and at least one of them contains another function of constructor call of any sort, force multi-line style for that outer call.

This is similar to what @munificent described with the nested list literal: the outer layers are forced to split, but the innermost instance can remain single-line.

All the nested widget builders given here would split, but Duration(hours: 1, minutes: 2) and EdgeInsets.only(top: 10.0, right: 20.0) would still fit a single line because they don't nest calls.

[wliumelb]

I believe everyone would agree that the list should always split if it contains an argument that requires Widget (e.g. child, home, title), List<Widget> (e.g. children, items) or Function (e.g. onTap, onPressed, onChanged, onFieldSubmitted)

That means that all of the formatting rules have to be syntactic in nature. The formatter knows what the code looks like, but it doesn't know what it means.

Is it possible to create a list of keywords that the formatter could read, so that if a argument list contains any one of these keywords, it will always split instead of following the default rule?

(Even if the formatter could analyze code, I still would prefer it to not rely on that when making formatting choices. It would be strange if the formatter did a great job with Flutter code, but a new framework comes along that looks and behaves just like Flutter but the formatter formats its code differently.)

If keyword-based rule is possible, then maybe the formatter could even allow the creator of a framework to supply their own keyword list to control how the formatted code should look like?

@munificent

[cabaucom376]

Is it possible to create a list of keywords

Personally I'm not a fan of this approach. I break those naming conventions with custom widgets on occasion. For example something I'm working on now instead of children the parameter is called items because that's what all the widgets in the package are called.

I think what @mernen and I are thinking of at least covers those cases implicitly as when you pass in a widget into a parameter, it's with its constructor.

[Nyaacinth]

I found that there's an ast extension property looksLikeStaticCall, though seems unreliable because that case covers 1) constructor call with or without new 2) named constructors and static methods that act like a constructor 3) (not-wanted) normal static calls. (I might be wrong since this is my very first time on exploring this) But that seems enough for #1647 (comment) 's implementation. I'm still trying to find a possible path for implementing this, though downgrading sdk version in pubspec is more practical in a short term.