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

Unintentional formatter deviation: strange spacing around TypeAlias declarations #8188

Closed
stinodego opened this issue Oct 24, 2023 · 3 comments · Fixed by #8233
Closed

Unintentional formatter deviation: strange spacing around TypeAlias declarations #8188

stinodego opened this issue Oct 24, 2023 · 3 comments · Fixed by #8233
Labels
formatter Related to the formatter needs-decision Awaiting a decision from a maintainer

Comments

@stinodego
Copy link
Contributor

stinodego commented Oct 24, 2023

Reproducible example:

from typing import TypeAlias

MultiColSelector: TypeAlias = (
    "slice | range | list[int] | list[str] | list[bool] | Series"
)

black leaves this alone, while ruff formats it as follows:

- MultiColSelector: TypeAlias = (
-     "slice | range | list[int] | list[str] | list[bool] | Series"
- )
+ MultiColSelector: (
+     TypeAlias
+ ) = "slice | range | list[int] | list[str] | list[bool] | Series"

This looks like an undesirable change to me.

In our code base, I worked around this by rewriting the string as a Union of types.

Command: ruff format .
Formatter settings: None
Version: ruff 0.1.2

P.S.: Other than the two minor issues I just reported, the new ruff formatter is working excellently. It has improved formatting in quite a few cases. Thank you for the amazing work!

@MichaReiser
Copy link
Member

Thank's for reporting and thank you for the kind words.

This deviation is documented as type annotations may be parenthesized and is related to 7315.

Let's keep this issue open to collect some more feedback as I can see that the formatting in this case is not ideal. I also want to re-test if implementing #6975 (preview style) will make fixing it obsolete.

@MichaReiser MichaReiser added formatter Related to the formatter needs-decision Awaiting a decision from a maintainer labels Oct 24, 2023
@MichaReiser MichaReiser added this to the Formatter: Stable milestone Oct 24, 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 |
@henryiii
Copy link
Contributor

AFAICT, the rule should be that type annotations are only parenthesized if the type annotation itself is long. It should never parenthesize a type annotation to make something after the equals sign have more space.

@charliermarsh
Copy link
Member

We ended up reverting this decision (so we no longer have this Black deviation). Once we implement preview style, we should have roughly the behavior you describe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
formatter Related to the formatter needs-decision Awaiting a decision from a maintainer
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants