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

Formatter undocumented deviation: slice formatting #7316

Open
JonathanPlasse opened this issue Sep 12, 2023 · 6 comments
Open

Formatter undocumented deviation: slice formatting #7316

JonathanPlasse opened this issue Sep 12, 2023 · 6 comments
Labels
formatter Related to the formatter style How should formatted code look

Comments

@JonathanPlasse
Copy link
Contributor

Black formatting

def f():
    for event_index in range(header.number_non_empty_events):
        section_header_data = struct.unpack(
            JSON_BINARY_PARAMETERS.fmt_section_header,
            byte_array[
                byte_begin_index
                + byte_step_index * event_index : byte_begin_index
                + byte_step_index * (event_index + 1)
            ],
        )

Ruff formatting

def f():
    for event_index in range(header.number_non_empty_events):
        section_header_data = struct.unpack(
            JSON_BINARY_PARAMETERS.fmt_section_header,
            byte_array[
                byte_begin_index + byte_step_index * event_index : byte_begin_index
                + byte_step_index * (event_index + 1)
            ],
        )

Use Ruff 0.0.289 with line length to 100

@zanieb
Copy link
Member

zanieb commented Sep 12, 2023

Do you have an ideal formatting for this case? I find both of these hard to reason about.

@JonathanPlasse
Copy link
Contributor Author

Maybe this

def f():
    for event_index in range(header.number_non_empty_events):
        section_header_data = struct.unpack(
            JSON_BINARY_PARAMETERS.fmt_section_header,
            byte_array[
                byte_begin_index + byte_step_index * event_index
                : byte_begin_index + byte_step_index * (event_index + 1)
            ],
        )

@MichaReiser
Copy link
Member

MichaReiser commented Sep 13, 2023

Thanks for reporting this. The deviation here is rooted in the fact that:

  • Black breaks a line by all binary operators first as if the left and right side of the : operator are a single expression
  • Ruff considered the left and right two independent expressions and split them individually

Here's an example where Black splits before all + operators even though it wouldn't be necessary to make the left side fit:

def f():
    for event_index in range(header.number_non_empty_events):
        section_header_data = struct.unpack(
            JSON_BINARY_PARAMETERS.fmt_section_header,
            byte_array[
                byte_begin_index
                + byte_step_index : byte_begin_index
                + byte_step_index
                + (event_index + 1)
            ],
        )

# Ruff
def f():
    for event_index in range(header.number_non_empty_events):
        section_header_data = struct.unpack(
            JSON_BINARY_PARAMETERS.fmt_section_header,
            byte_array[
                byte_begin_index + byte_step_index : byte_begin_index
                + byte_step_index
                + (event_index + 1)
            ],
        )

I generally prefer Ruff's approach of treating them as separate expressions but we should probably add a softline break before the : operator similar to other operands (e.g. in binary expressions) and potentially indent the right side.

Prettier's formatting looks similar to what @JonathanPlasse proposes (I had to adjust the syntax slightly to get the JS equivalent)

for (event_index in range(header.number_non_empty_events)) {
  section_header_data = struct.unpack(
    JSON_BINARY_PARAMETERS.fmt_section_header,
    {
      [byte_begin_index + byte_step_index * event_index]:
        byte_begin_index + byte_step_index * (event_index + 1),
    },
  );
}

@MichaReiser MichaReiser added wontfix This will not be worked on formatter Related to the formatter labels Sep 13, 2023
@MichaReiser MichaReiser added this to the Formatter: Beta milestone Sep 13, 2023
konstin added a commit that referenced this issue Sep 13, 2023
**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 added a commit that referenced this issue Sep 13, 2023
**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 konstin added wontfix This will not be worked on and removed wontfix This will not be worked on labels Sep 13, 2023
@MichaReiser
Copy link
Member

We decided not to implement the deviation for now and instead focus on Black compatibility because:

  • It eases migration for Black users
  • Having more deviation makes it harder to assess whether a deviation is a bug or due to som other deviation. The fewer deviations, the easier it is to identify the root cause and improve overall black compatibility

We plan to re-visit the formatting eventually

@charliermarsh
Copy link
Member

We're not planning to change this for the Beta. We want to re-open the possibility of changing to match @JonathanPlasse's suggested style once we have better tooling to evaluate the impact (e.g., ruff-shades).

@MichaReiser MichaReiser added wontfix This will not be worked on style How should formatted code look and removed wontfix This will not be worked on labels Oct 26, 2023
@MichaReiser MichaReiser removed this from the Formatter: Stable milestone Oct 26, 2023
@MichaReiser MichaReiser removed the wontfix This will not be worked on label Oct 27, 2023
@MichaReiser
Copy link
Member

We may want to consider #8897 when making a style decision to make sure our handling is consistent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
formatter Related to the formatter style How should formatted code look
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants