-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Conversation
There was a problem hiding this 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(),] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[target.format(), token(":"), space(), annotation.format(),] | |
[target.format(), token(":"), space(), annotation.format()] |
810f53a
to
533e4dd
Compare
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. |
533e4dd
to
ede63e9
Compare
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:
After: