Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Indent block-like function expression bodies when split at => #1202

Closed
munificent opened this issue Apr 4, 2023 · 1 comment
Closed

Indent block-like function expression bodies when split at => #1202

munificent opened this issue Apr 4, 2023 · 1 comment

Comments

@munificent
Copy link
Member

A function expression may have a => body followed by a block-like expression, like a list, map, switch expression, or even another function expression. Right now, that inner expression's block indentation is not increased by virtue of being nested inside a function expression. That leads to output like:

SomeLongFunctionName(
  argument: () => [
    listElement,
    listElement,
    listElement,
  ],
);

I think this formatting works pretty well. Most users seem to be OK with it too since lots of code is formatted this way and I haven't heard many complaints. However, the formatting looks weird if we end up needing to split after the => because the parameter list is long or before the block-like expression has something long before its opening bracket, as in:

SomeLongFunctionName(
  argument: (longParameterName______) =>
      <LongTypeArgument>[
    listElement,
    listElement,
    listElement,
  ],
);

This looks pretty strange because now the opening delimiter is on its own line (as opposed to be on the same line as the =>) but that line doesn't align with the corresponding closing delimiter. It looks particularly strange with a switch expression (#1197):

SomeLongFunctionName(
  argument: (longParameterName______) =>
      switch (e) {
    1 => listElement,
    2 => listElement,
    3 => listElement,
  },
);

I've tried changing it to always indent the block body, which gives:

SomeLongFunctionName(
  argument: (longParameterName______) =>
      switch (e) {
        1 => listElement,
        2 => listElement,
        3 => listElement,
      },
);

This example now looks better, but the first one is worst:

SomeLongFunctionName(
  argument: () => [
        listElement,
        listElement,
        listElement,
      ],
);

From testing on a corpus of open source code, there are a lot of callsites that would be affected by this, and almost all of them ended up looking worse like the last example here.

The correct fix is to have the block indentation change based on whether we split after the =>. If it splits, then indent the block, otherwise don't.

Unfortunately, the architecture of the formatter doesn't currently support that. Block indentation is calculated when the AST is transformed into chunks before line splitting.

I won't be able to fix that easily, so I'm filing this to track hopefully being able to do that better fix when I have more time.

munificent added a commit that referenced this issue Apr 4, 2023
This makes them all line up as if they were empty cases in a switch
statement, which is essentially how they behave. Instead of:

```dart
e = switch (obj) {
  constant1 ||
     constant2 ||
     constant3 =>
    body
};
```

Gives:

```dart
e = switch (obj) {
  constant1 ||
  constant2 ||
  constant3 =>
    body
};
```

This addresses some of the bad formatting in #1197. The rest is #1202.
munificent added a commit that referenced this issue Apr 4, 2023
…es. (#1203)

* Don't indent outermost "||" pattern operands in switch expression cases.

This makes them all line up as if they were empty cases in a switch
statement, which is essentially how they behave. Instead of:

```dart
e = switch (obj) {
  constant1 ||
     constant2 ||
     constant3 =>
    body
};
```

Gives:

```dart
e = switch (obj) {
  constant1 ||
  constant2 ||
  constant3 =>
    body
};
```

This addresses some of the bad formatting in #1197. The rest is #1202.

* Add comment.
@munificent
Copy link
Member Author

I ran these examples through the forthcoming tall style and they look like:

SomeLongFunctionName(
  argument:
      () => [
        listElement,
        listElement,
        listElement,
      ],
);

SomeLongFunctionName(
  argument:
      (longParameterName______) =>
          <LongTypeArgument>[
            listElement,
            listElement,
            listElement,
          ],
);

SomeLongFunctionName(
  argument:
      (longParameterName______) =>
          switch (e) {
            1 => listElement,
            2 => listElement,
            3 => listElement,
          },
);

The new formatting style doesn't try to keep the parameter list and => on the same line as a named argument. It has a simpler more regular set of rules for deciding to split there. So this output isn't quite what this issue initially talks about, but I think the output makes sense and looks clear. The most important part is that it never has indentation that looks "backwards" where the collection literal or switch body is indented less than the closure than contains it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant