Indent ternary operands beneath the operator #1534
Comments
|
Yeah, I agree this doesn't look great. |
munificent
changed the title
munificent
added a commit
that referenced
this issue
Dec 17, 2024
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
I think it's weird that the arguments for
Uri.dataFromStringhere are indented at the same level as the function call:I'd prefer
...which I think also better matches the way constructor initializers are indented.
The text was updated successfully, but these errors were encountered: