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

Avoid introducing new parentheses in annotated assignments #8233

Merged
merged 1 commit into from Oct 26, 2023

Conversation

charliermarsh
Copy link
Member

@charliermarsh charliermarsh commented Oct 25, 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

@charliermarsh charliermarsh added the formatter Related to the formatter label Oct 25, 2023
Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should update the documentation too.

Are there type annotations in other positions that need updating too?

I'm somewhat torn on this (see comment in #8188) because I think the unfortunate formatting will be fixed when implementing the preview style. Also, there are now cases (with unions on the left), that now exceed the line width, but didn't before. Should we special case instead to only avoid parenthesizing if the type annotation is a single name?

Could we gate the old behavior behind the preview flag? I think the old formatting yields better results if we parenthesize the left, if parenthesizing the right didn't make it fit.

space(),
maybe_parenthesize_expression(annotation, item, Parenthesize::IfBreaks)
]
[target.format(), token(":"), space(), annotation.format(),]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
[target.format(), token(":"), space(), annotation.format(),]
[target.format(), token(":"), space(), annotation.format()]

@charliermarsh
Copy link
Member Author

Went ahead and updated the docs. My current thinking here is that we should change this based on improving Black compatibility. The current formatting is better in some cases, e.g., from the docs:

# Black
StartElementHandler: Callable[[str, dict[str, str]], Any] | Callable[[str, list[str]], Any] | Callable[
    [str, dict[str, str], list[str]], Any
] | None

# Ruff
StartElementHandler: (
    Callable[[str, dict[str, str]], Any]
    | Callable[[str, list[str]], Any]
    | Callable[[str, dict[str, str], list[str]], Any]
    | None
)

But I think we can improve those by making progress on preview style -- and complex union annotations are also much rarer than simple annotations.

@charliermarsh charliermarsh merged commit 88c8b47 into main Oct 26, 2023
17 checks passed
@charliermarsh charliermarsh deleted the charlie/ann-assign branch October 26, 2023 02:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
formatter Related to the formatter
Projects
None yet
2 participants