Break before slice colon #7332
Closed
Break before slice colon #7332
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Nice! Does this impact our similarity index? How would it look if we intended the right side if the line breaks? |
MichaReiser
reviewed
Sep 13, 2023
| @@ -91,7 +91,7 @@ impl FormatNodeRule<ExprSlice> for FormatExprSlice { | |||
| if !all_simple && lower.is_some() { | |||
| space().fmt(f)?; | |||
| } | |||
| token(":").fmt(f)?; | |||
| write!(f, [soft_line_break(), token(":")])?; | |||
There was a problem hiding this comment.
We should move this into the else branch or we may end up with an unintentional a \n<space>
MichaReiser
reviewed
Sep 13, 2023
| @@ -155,7 +167,8 @@ d1 = "d"[ # comment | |||
| : | |||
| ] | |||
| d2 = "d"[ # comment | |||
| 1: | |||
| 1 | |||
| : | |||
There was a problem hiding this comment.
Hmm, this isn't ideal. Do we need to group the line break with the left side so that it only renders if the left expands (or the : and the right side don't fit on the line)
**Summary** Break slices at the colon first, since the colon is separator with the lowest precedence and we're in a parenthesized context.
**Input**
```python
section_header_data = byte_array[byte_begin_index + byte_step_index * event_index : byte_begin_index + byte_step_index * (event_index + 1)]
```
**Black**
```python
section_header_data = byte_array[
byte_begin_index
+ byte_step_index * event_index : byte_begin_index
+ byte_step_index * (event_index + 1)
]
```
**Current formatting**
```python
section_header_data = byte_array[
byte_begin_index + byte_step_index * event_index : byte_begin_index
+ byte_step_index * (event_index + 1)
]
```
**Proposed formatting**
```python
section_header_data = byte_array[
byte_begin_index + byte_step_index * event_index
: byte_begin_index + byte_step_index * (event_index + 1)
]
```
This is another intentional black deviation, but i find it a clear style improvement.
This is consistent with adding a step:
```python
section_header_data2 = byte_array[
byte_begin_index + byte_step_index * event_index
: byte_begin_index + byte_step_index
: section_size
]
```
As-is, this regresses trailing colon comments:
**in**
```python
c1 = "c"[
1: # e
# f
2
]
```
**out**
```python
c1 = "c"[
1
: # e
# f
2
]
```
Fixes #7316
**Test Plan** Added the fixtures above.
konstin
force-pushed
the
break-before-slice-colon
branch
from
e5f4163 to
d4cfe1f
Compare
|
We have decided to not merge this to instead focus on black compatibility instead of adding more deviations |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary Break slices at the colon first, since the colon is separator with the lowest precedence and we're in a parenthesized context.
Input
Black
Current formatting
Proposed formatting
This is another intentional black deviation, but i find it a clear style improvement.
This is consistent with adding a step:
As-is, this regresses trailing colon comments:
in
out
main
PR
For the similarity index, this is a regression.
Fixes #7316
Test Plan Added the fixtures above.