New style: Minor change causes huge whitespace diff

[goderbauer]

The code below was formatted before I changed a minor detail and then formatted again. Can you spot what the actual non-whitespace change is?

(The change is that a key param was added to the Stack widget)

These kind of dramatic diffs for a minor code change are going to make code reviews harder.

[jakemac53]

Have you tried viewing this diff without whitespace diffs? Github supports that and it usually works pretty well.

[johnmccutchan]

+1 to @jakemac53 this is actually a UI issue with GitHub's diff viewer

[johnmccutchan]

Also, why is the diff viewer configured to show the unified diff and not split view?

[jakemac53]

Here is a diff for the build repo with the new formatter and whitespace disabled https://github.com/dart-lang/build/compare/tall-style?w=1. It seems to mostly get it right (primarily you are seeing the added commas).

[munificent]

There are a few places where these kinds of "small source change leads to large format change" behaviors can happen. It is something I'm mindful of when designing the style rules, but I have to balance that against the goal of having formatted output that users like.

Another example is that the new formatting style lines up constructor initializers:

class SomeClass {
  SomeClass(
    int someParameter,
    int anotherParameter,
  ) : _firstField = 1,
      _secondField = 2,
      _thirdField = 3,
      _fourthField = 4;
}

The subsequent initializers are spaced to align with the first one. But that means that if you change the first initializer's indentation by making the parameters named or optional, all of the initializers move over a space:

class SomeClass {
  SomeClass({
    int someParameter,
    int anotherParameter,
  }) : _firstField = 1,
       _secondField = 2, // < Indented +1 more.
       _thirdField = 3, // < Indented +1 more.
       _fourthField = 4; // < Indented +1 more.
}

This is particularly odd because the change is in the parameter list but it affects the initializers, which are a totally separate construct. I don't love that style but... if you look in the Flutter repo, this is how the majority of constructors are hand formatted. So at least going by the Flutter team's observed behavior, it seems like a pleasant style matters more than minimizing diff churn.

So, yes, your concern is valid, but I think it's a price our users are OK with paying in most cases as far as I can tell.

[goderbauer]

I think it's a price our users are OK with paying

What am I paying that price FOR, though? What do I get in return here? In other words, why does the formatter prefer formatting single-arguments list as follows:

return Stack(children: <Widget>[
  Positioned(
    top: anchor.dy,
    left: anchor.dx,
    child: Container(
      width: 200.0,
      height: 200.0,
      color: Colors.cyanAccent.withOpacity(0.5),
      child: GridView.count(
        padding: const EdgeInsets.all(12.0),
        crossAxisCount: 2,
        children: children,
      ),
    ),
  ),
]);

... instead of doing it this way (which would be consistent with how multi-argument lists are formatted):

return Stack(
  children: <Widget>[
    Positioned(
      top: anchor.dy,
      left: anchor.dx,
      child: Container(
        width: 200.0,
        height: 200.0,
        color: Colors.cyanAccent.withOpacity(0.5),
        child: GridView.count(
          padding: const EdgeInsets.all(12.0),
          crossAxisCount: 2,
          children: children,
        ),
      ),
    ),
  ],
);

In the latter case, there is "no price to pay" when adding another parameter. So, what makes the former preferable that this price is worth it?

I do understand that there are always trade-offs to be made in code formatting. I am just wondering what the trade-off here is since this is a very common Flutter coding pattern.

[jakemac53]

why does the formatter prefer formatting single-arguments list as follows:

As an outside observer - what you get is less indentation. For highly nested build methods decreasing the indentation is highly beneficial imo.

[johnmccutchan]

I think it's a price our users are OK with paying

What am I paying that price FOR, though? What do I get in return here?

There will never be an automatic formatter that gets everything perfect according to everyone's subjective opinions on what good code aesthetics are.

You get to stop thinking about formatting. You get to stop manually formatting code. You no longer have to nag outside contributors to adjust white space in their PRs.

These are the reasons why we are adopting the formatter.

Judging code formatting is a subjective experience.

To my eyes both options you presented look fine to me.

The difference between the two seems small and wouldn't impact my ability to read or modify that code.