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

Support "block argument" formatting. #1322

Merged
merged 3 commits into from
Nov 17, 2023
Merged

Support "block argument" formatting. #1322

merged 3 commits into from
Nov 17, 2023

Conversation

munificent
Copy link
Member

@munificent munificent commented Nov 15, 2023

Sometimes in an argument list, we don't want to force it to full split even if an argument contains an inner newline. In other words, we want to prefer:

test('hi', () {
  body;
});

Over:

test(
  'hi',
  () {
    body;
  },
);

The latter is what you would normally get when an argument contains a newline. This change enables the former. The heuristics for what kinds of arguments are allowed to have this block-like formatting, how many of them, and what other arguments are allowed are pretty fuzzy.

I picked a simple set of rules here (which follow the prototype), but left some TODOs because I suspect we will iterate on them later once we can run the new formatter on larger codebases.

The heuristic for what kinds of arguments are block-like is the same rule that we use for "delimited" expressions after an assignment. I think it's good that those two agree and helps make the style overall more consistent. Basically, any time the right-hand side of an = or : doesn't need to split, that same kind of expression can be block formatted:

var list = [
  element,
];

function([
  element,
]);

Block formatting is used for argument lists, asserts, and switch values.

I also simplified AssignPiece. It used an extra unnecessary to allow newlines inside delimited value expressions without splitting after "=".

Sometimes in an argument list, we don't want to force it to full split
even if an argument contains an inner newline. In other words, we want
to prefer:

```
test('hi', () {
  body;
});
```

Over:

```
test(
  'hi',
  () {
    body;
  },
);
```

The latter is what you would normally get when an argument contains a
newline. This change enables the former. The heuristics for what kinds
of arguments are allowed to have this block-like formatting, how many
of them, and what other arguments are allowed are pretty fuzzy.

I picked a simple set of rules here (which follow the prototype), but
left some TODOs because I suspect we will iterate on them later once we
can run the new formatter on larger codebases.

The heuristic for what kinds of arguments are block-like is the same
rule that we use for "delimited" expressions after an assignment. I
think it's good that those two agree and helps make the style overall
more consistent. Basically, any time the right-hand side of an `=` or
`:` doesn't need to split, that same kind of expression can be block
formatted:

```
var list = [
  element,
];

function([
  element,
]);
```

Block formatting is used for argument lists, asserts, and switch values.

I also simplified AssignPiece. It used an extra unnecessary to allow
newlines inside delimited value expressions without splitting after "=".
@munificent
Copy link
Member Author

This is a pretty big one, but it's a big step towards actually producing the output we want. Also, it's another example of the kind of formatting that the new formatter can handle but the old one can't. In the old formatter, if an argument could possibly be block formatted, it gets locked into that indentation. If it turns out that the block formatting doesn't fit, you end up with weird indentation like:

function(
    'some string', <SomeLongType>[
  element,
  element,
  element
]);

With this change in the new formatter, you get:

function(
  'some string',
  <SomeLongType>[
    element,
    element,
    element,
  ],
);

I've wanted to fix this for a loooong time.

lib/src/ast_extensions.dart Outdated Show resolved Hide resolved
lib/src/ast_extensions.dart Outdated Show resolved Hide resolved
lib/src/ast_extensions.dart Outdated Show resolved Hide resolved
Also renamed "isDelimited" (never a great name) to "canBlockSplit",
which I think better captures its use.
Copy link
Member

@kallentu kallentu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@munificent munificent merged commit 28fa4be into main Nov 17, 2023
7 checks passed
@munificent munificent deleted the block-arguments branch November 17, 2023 23:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants