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: annotation wrapping #7315

Closed
JonathanPlasse opened this issue Sep 12, 2023 · 3 comments · Fixed by #7679
Closed

Formatter undocumented deviation: annotation wrapping #7315

JonathanPlasse opened this issue Sep 12, 2023 · 3 comments · Fixed by #7679
Assignees
Labels
documentation Improvements or additions to documentation formatter Related to the formatter

Comments

@JonathanPlasse
Copy link
Contributor

Black formatting

bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb: Bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb = (
    Bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb()
)

Ruff formatting

bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb: (
    Bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
) = Bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb()

Use ruff 0.0.289.
Both are set to a line length of 100.

@MichaReiser
Copy link
Member

MichaReiser commented Sep 13, 2023

Thanks for reporting this deviation. Black seems to preserve parentheses in type annotation positions without ever introducing new parentheses.

This is inconsistent with other places where expression appear as a direct child of a statement (other than expression statement) where Black prefers to strip any unnecessary parentheses (and applies the custom "can omit parentheses" layout).

This can be solved "easily" by changing this line to use annotation.format() instead of maybe_parenthesize but it means that Ruff is more likely to exceed the line width

write!(
f,
[
target.format(),
token(":"),
space(),
maybe_parenthesize_expression(annotation, item, Parenthesize::IfBreaks)
]
)?;

Changing the line accordingly improves the similarity index for typeshed by 0.00003

This is probably related to #7322

@MichaReiser
Copy link
Member

Copied over from #7322

I can see how Black's formatting may seem better but Black's code exceeds the configured line width of 100, whereas Ruff's formatting stays in the limit.

Black is also a bit inconsistent here because it uses the same formatting as ruff if you change this to an assignment:

animation_data = (
    bpy.types.AnimData
)  # TODO(jonathan): Change to Optional when fake-bpy-module is updated

I propose that we stick to Ruff's layout because:

  • It is more consistent with other syntax
  • It stays within the configured line length

I'll leave this open for now to let other people chime in.

@MichaReiser MichaReiser added formatter Related to the formatter needs-decision Awaiting a decision from a maintainer labels Sep 13, 2023
@MichaReiser MichaReiser added this to the Formatter: Beta milestone Sep 13, 2023
@MichaReiser MichaReiser removed the needs-decision Awaiting a decision from a maintainer label Sep 27, 2023
@charliermarsh
Copy link
Member

This is a known deviation -- can be closed once it's documented.

@charliermarsh charliermarsh self-assigned this Sep 27, 2023
@MichaReiser MichaReiser added the documentation Improvements or additions to documentation label Sep 28, 2023
charliermarsh added a commit that referenced this issue Oct 26, 2023
## Summary

We decided to avoid changing this in
#7315, but it's been reported
multiple times (e.g., in #8226,
also on Discord). I suggest we change it to improve compatibility. In
general, it also seems to lend itself to better code style.

Closes #8188 
Closes #8226

## Test Plan

Shows improvements for CPython, home-assistant, Poetry, and typeshed.

Before:

| project | similarity index | total files | changed files |

|----------------|------------------:|------------------:|------------------:|
| cpython | 0.75803 | 1799 | 1647 |
| django | 0.99983 | 2772 | 34 |
| home-assistant | 0.99953 | 10596 | 186 |
| poetry | 0.99891 | 317 | 17 |
| transformers | 0.99966 | 2657 | 330 |
| twine | 1.00000 | 33 | 0 |
| typeshed | 0.99978 | 3669 | 20 |
| warehouse | 0.99977 | 654 | 13 |
| zulip | 0.99970 | 1459 | 22 |

After:

| project | similarity index | total files | changed files |

|----------------|------------------:|------------------:|------------------:|
| cpython | 0.75804 | 1799 | 1647 |
| django | 0.99983 | 2772 | 34 |
| home-assistant | 0.99960 | 10596 | 156 |
| poetry | 0.99897 | 317 | 17 |
| transformers | 0.99966 | 2657 | 330 |
| twine | 1.00000 | 33 | 0 |
| typeshed | 0.99980 | 3669 | 18 |
| warehouse | 0.99977 | 654 | 13 |
| zulip | 0.99970 | 1459 | 22 |
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation formatter Related to the formatter
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants