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

Format adjacent strings. #1364

Merged
merged 4 commits into from
Jan 23, 2024
Merged

Format adjacent strings. #1364

merged 4 commits into from
Jan 23, 2024

Conversation

munificent
Copy link
Member

@munificent munificent commented Jan 23, 2024

Most of this was fairly straightforward, but the two tricky bits are:

1. Deciding whether or not to indent

There are some rules around whether subsequent strings in the adjacent strings get indented or not. The answer is yes in some cases to avoid confusion:

var list = function(
  'string 1',
  'adjacent'
      'string 2',
  'string 3',
];

But not in others since it looks nicer to line them up when possible:

var description =
    'some text '
    'more text';

2. Handling test() and group()

It's really important that test functions don't end up fully split because doing so would lead to the inner function expression getting indented +2:

test('this looks good', () {
  body;
});

test(
  'this looks bad',
  () {
    body;
  },
);

Test descriptions often span multiple lines using adjacent strings:

test('this is a very long test description '
    'spanning multiple lines, () {
  body;
});

Normally, the newline inside the adjacent strings would cause the entire argument list to split. The old style handles that (I think) by allowing multiple block-formatted arguments and then treating both the adjacent strings and the function expressions as block arguments.

The new style currently only allows a single block argument (because in almost all of the Flutter code I found using block formatting, one argument was sufficient). So I chose a more narrowly targeted rule here where we allow adjacent strings to not prevent block formatting only if the adjacent strings are the first argument and the block argument is a function expression as the next argument.

I left a TODO to see if we want to iterate on that rule, but I think it works pretty well.

Other stuff

Unlike the old style, I chose to always split between adjacent strings. The old style will preserve newlines there but if a user chooses to deliberately put multiple adjacent strings on the same line and they fit, it will honor it. That didn't seem useful to me, so now they just always split. I don't think adjacent strings ever look good on the same line.

I ended up moving the state to track which elements in a ListPiece out of ListPiece and into the ListElements themselves. I think it's clearer this way and will be easier to evolve if we end up supporting multiple block formatted elements in a single list.

Most of this was fairly straightforward, but the two tricky bits are:

### 1. Deciding whether or not to indent

There are some rules around whether subsequent strings in the adjacent
strings get indented or not. The answer is yes in some cases to avoid
confusion:

```
var list = function(
  'string 1',
  'adjacent'
      'string 2',
  'string 3',
];
```

But not in others since it looks nicer to line them up when possible:

```
var description =
    'some text '
    'more text';
```

### 2. Handling `test()` and `group()`

It's really important that test functions don't end up fully split
because doing so would lead to the inner function expression getting
indented +2:

```
test('this looks good', () {
  body;
});

test(
  'this looks bad',
  () {
    body;
  },
);
```

Test descriptions often span multiple lines using adjacent strings:

```
test('this is a very long test description '
    'spanning multiple lines, () {
  body;
});
```

Normally, the newline inside the adjacent strings would cause the entire
argument list to split. The old style handles that (I think) by allowing
multiple block-formatted arguments and then treating both the adjacent
strings and the function expressions as block arguments.

The new style currently only allows a single block argument (because in
almost all of the Flutter code I found using block formatting, one
argument was sufficient). So I chose a more narrowly targeted rule here
where we allow adjacent strings to not prevent block formatting only if
the adjacent strings are the first argument and the block argument is a
function expression as the next argument.

I left a TODO to see if we want to iterate on that rule, but I think it
works pretty well.

### Other stuff

Unlike the old style, I chose to always split between adjacent strings.
The old style will preserve newlines there but if a user chooses to
deliberately put multiple adjacent strings on the same line and they
fit, it will honor it. That didn't seem useful to me, so now they just
always split. I don't think adjacent strings ever look good on the same
line.

I ended up moving the state to track which elements in a ListPiece out
of ListPiece and into the ListElements themselves. I think it's clearer
this way and will be easier to evolve if we end up supporting multiple
block formatted elements in a single list.
@srawlins
Copy link
Member

Unlike the old style, I chose to always split between adjacent strings.

Sounds like this fixes #684. Not sure how bugs are being tracked between new and existing formatter...

lib/src/front_end/delimited_list_builder.dart Outdated Show resolved Hide resolved
lib/src/front_end/delimited_list_builder.dart Outdated Show resolved Hide resolved
lib/src/front_end/delimited_list_builder.dart Outdated Show resolved Hide resolved
lib/src/piece/adjacent_strings.dart Outdated Show resolved Hide resolved
lib/src/front_end/delimited_list_builder.dart Outdated Show resolved Hide resolved
lib/src/front_end/delimited_list_builder.dart Outdated Show resolved Hide resolved
@munificent
Copy link
Member Author

Sounds like this fixes #684. Not sure how bugs are being tracked between new and existing formatter...

I haven't really figured that out either. :) I'll probably just do a pass through the issue tracker once we've moved to the new style and close anything that's done. It feels weird to close them now because the new style is hidden behind an experiment flag.

@munificent munificent merged commit e697b6e into main Jan 23, 2024
7 checks passed
@munificent munificent deleted the format-adjacent-strings branch January 23, 2024 23:01
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.

4 participants