Revamp conditional expression indentation. #1626
Conversation
This PR makes two changes to how conditional expressions are indented:
**Don't indent subsequent conditional branches if an assignment context.**
The formatter has some special handling for infix operators inside assignment-like contexts (after `=`, `:`, or `=>`) to avoid otherwise redundant-seeming indentation. Normally, you'd get:
```dart
variable =
operand +
another;
```
The +4 before `operand +` is from the assignment indenting the RHS. And then the +4 before `another;` is that plus the +4 indentation from the binary operator. It looks better if the operands line up, so when a binary operator is in an assignment context, we don't indent it. That gives:
```dart
variable =
operand +
another;
```
I think that looks nice, and users have asked for it. For no good reason, when writing the new formatter we didn't do that for conditional expressions. You get:
```dart
variable =
condition
? thenBranch
: elseBranch;
```
This PR applies the same rule to conditionals that we do for binary operators, giving:
```dart
variable =
condition
? thenBranch
: elseBranch;
```
**Indent then and else branches past their operator.**
If there's a split *inside* a conditional operator's operand, we have to decide how to indent it. Currently, there is no indentation at all. That looks OK if the internal split is from something like a binary operator:
```dart
condition
? longOperand +
anotherLongOperand
: thirdOperand +
anotherLongOperand;
```
But it looks less good if the split is something like a function call:
```dart
condition
? function(
argument,
another,
)
: function(
argument,
another,
);
```
It seems weird that the `)` are at the same indentation as the conditional operators themselves.
This PR fixes #1534 and indents the then and else branches (but not the condition) two spaces past the operands. It does this for all operand subexpressions, giving:
```dart
condition
? longOperand +
anotherLongOperand
: function(
argument,
another,
);
```
**Together:**
If you put these changes together, then in an assignment context, the `?` and `:` move fours spaces left and their subexpressions move two spaces left:
```dart
// Before:
SomeWidget(
child:
condition
? AnotherWidget(
argument,
argument,
)
: ThirdWidget(
argument,
argument,
)
);
// After:
SomeWidget(
child:
condition
? AnotherWidget(
argument,
argument,
)
: ThirdWidget(
argument,
argument,
)
);
```
The overall effect is to move conditionals to the left. That in turn means some code that would wrap no longer has to. This ends up being pretty valuable because conditional expressions are very common in Flutter widget code since we don't allow `if` inside argument lists.
I've tested this on a big corpus of pub packages and every diff I saw looked better to my eyes.
I'd like to get this change in before 3.7 ships on stable so that users don't have to deal with the churn if possible. At the same time, we've already migrated the Dart SDK, internal code, and much of Flutter, so it would be somewhat disruptive.
If we have to wait until after 3.7 and ship in 3.8, we can do that too.
|
The change itself looks reasonable. In terms of disruption, I measured for internal code: it affects 11.5% of files. So not as big as the reformat we already did, I think it's landable, but we'd have to plan the work and do it quickly as the new version rolls in to minimize disruption. As before we'd need a few more people signed up to make that happen. Are there any other changes this could be bundled with? In cases where they touch the same files, that's pretty much free. Happy to measure if you have any WIP changes. |
|
Either it should land in the SDK this week, over the holidays (if there's someone that can deal with the re-formatting) or after the cut-off on Jan 8th. |
👍
Wow, more than I expected.
Do we need to do a migration? In the past with style changes, we've just landed them and let users reformat as they touch files. I can't recall anyone complaining, though it has been a while and the style changes back then tended to be pretty minor.
Yes, there are a handful of other minor style fixes I've landed on the main branch here that would be rolled in too. They are:
Except for #1602, those are all extremely edge case and shouldn't affect much code.
Yes, I would aim to roll it into the SDK today or tomorrow. Some unfortunate timing: I will be out for two weeks for the holidays (12/23 - 1/3) and then getting surgery on 1/7 and out some unknown but hopefully small number of days after that. |
|
There's no way to roll it into google3 on that timeline; it would be necessary to pause rolls. |
|
We posted simultaneously, let me also reply to your question.
I think we'd definitely get complaints for 11.5% of files; it's enough impact that I guess it needs review+discussion. |
OK, I'll start an email. |
|
Overall this looks like an improvement. I am happy to coordinate rolling this into flutter/flutter. No preference for weather to land this before or after the cutoff. |
|
We spent some time discussing this and decided that given we're in the holiday season and the 3.7 cutoff is so close, the best thing is to land this change in 3.8. So I'll let this PR sit for a while until we've rolled the last version of dart_style into the SDK and then it will go in after that. |
This PR makes two changes to how conditional expressions are indented.
Don't indent subsequent conditional branches if an assignment context:
The formatter has some special handling for infix operators inside assignment-like contexts (after
=,:, or=>) to avoid otherwise redundant-seeming indentation. Normally, you'd get:The +4 before
operand +is from the assignment indenting the RHS. The +8 beforeanother;is the +4 from the assignment plus the +4 indentation from the binary operator. It looks better if the operands line up, so when a binary operator is in an assignment context, we don't indent it. That gives:I think that looks nice, and users have asked for it. For no good reason, when writing the new formatter we didn't do that for conditional expressions. Instead, you get:
This PR applies the same rule to conditionals that we do for binary operators, giving:
Indent then and else branches past their operator:
If there's a split inside a conditional operator's operand, we have to decide how to indent it. Currently, there is no indentation at all. That looks OK if the internal split is from something like a binary operator:
condition ? longOperand + anotherLongOperand : thirdOperand + anotherLongOperand;But it looks less good if the split is something like a function call:
condition ? function( argument, another, ) : function( argument, another, );It seems weird that the
)are at the same indentation as the conditional operators themselves, like the operands are trying to escape the conditional expression.This PR fixes #1534 and indents the then and else branches (but not the condition) two spaces past the operands. It does this for all operand subexpressions, giving:
condition ? longOperand + anotherLongOperand : function( argument, another, );Together:
If you put these changes together, then in an assignment context, the
?and:move fours spaces left and their subexpressions move two spaces left:The overall effect is to move conditionals to the left. That in turn means some code that would wrap no longer has to. This ends up being pretty valuable because conditional expressions are very common in Flutter widget code since we don't allow
ifinside argument lists.I've tested this on a big corpus of pub packages and every diff I saw looked better to my eyes. If you look at the test diffs you'll see what I mean.
I'd like to get this change in before 3.7 ships on stable so that users don't have to deal with the churn if possible. At the same time, we've already migrated the Dart SDK, internal code, and much of Flutter, so it would be somewhat disruptive.
If we have to wait until after 3.7 and ship in 3.8, we can do that too.
Thoughts on whether/when we should land this @natebosch, @davidmorgan, @goderbauer, @athomas, @mit-mit?