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

RUF022, RUF023: Ensure closing parentheses for multiline sequences are always on their own line #9793

Merged
merged 4 commits into from
Feb 9, 2024

Conversation

AlexWaygood
Copy link
Member

Summary

Currently these rules apply the heuristic that if the original sequence doesn't have a newline in between the final sequence item and the closing parenthesis, the autofix won't add one for you. The feedback from @ThiefMaster, however, was that this was producing slightly unusual formatting -- things like this:

__all__ = [
    "b", "c",
    "a", "d"]

were being autofixed to this:

__all__ = [
    "a",
    "b",
    "c",
    "d"]

When, if it was going to be exploded anyway, they'd prefer something like this (with the closing parenthesis on its own line):

__all__ = [
    "a",
    "b",
    "c",
    "d"
]

I'm still pretty skeptical that we'll be able to please everybody here with the formatting choices we make; but, on the other hand, this specific change is pretty easy to make.

Test Plan

cargo test. I also ran the autofixes for RUF022 and RUF023 on CPython to check how they looked; they looked fine to me.

Copy link
Contributor

github-actions bot commented Feb 2, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@charliermarsh charliermarsh self-requested a review February 9, 2024 14:06
@charliermarsh charliermarsh added the rule Implementing or modifying a lint rule label Feb 9, 2024
// and so that the final item has a trailing comma.
// This produces formatting more similar
// to that which the formatter would produce.
let parenthesis_re = regex::Regex::new(r"^,?([\])}])$").unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to write this via string find / filter checks, rather than using a regex? Regex tends to be overkill and slower for simple operations like this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, definitely doable -- just slightly less elegant here :)

I'll make the change!

Copy link
Member

Choose a reason for hiding this comment

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

As a tip: if we did use a regex, we should ensure that it's only compiled once (like in Python). It can make a huge difference. You can grep for Lazy<Regex> as an example of that pattern.

Copy link
Member Author

Choose a reason for hiding this comment

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

Got rid of the regex in 6fa2c43!

Copy link

codspeed-hq bot commented Feb 9, 2024

CodSpeed Performance Report

Merging #9793 will improve performances by 44.39%

Comparing AlexWaygood:closing-paren (6fa2c43) with main (c53aae0)

Summary

⚡ 18 improvements
✅ 12 untouched benchmarks

Benchmarks breakdown

Benchmark main AlexWaygood:closing-paren Change
parser[numpy/globals.py] 1,117.4 µs 963.5 µs +15.98%
parser[large/dataset.py] 69.1 ms 63.5 ms +8.78%
lexer[numpy/globals.py] 223.5 µs 154.8 µs +44.39%
linter/all-rules[numpy/globals.py] 2.8 ms 2.6 ms +7.65%
lexer[unicode/pypinyin.py] 571.5 µs 526.5 µs +8.55%
linter/all-rules[pydantic/types.py] 44.3 ms 39.8 ms +11.4%
lexer[numpy/ctypeslib.py] 1.8 ms 1.6 ms +15.26%
linter/all-rules[unicode/pypinyin.py] 11.8 ms 10.2 ms +15.65%
parser[pydantic/types.py] 26.4 ms 24.4 ms +8.1%
parser[unicode/pypinyin.py] 4.3 ms 3.9 ms +9.89%
linter/all-rules[numpy/ctypeslib.py] 22 ms 18.9 ms +16.46%
linter/all-with-preview-rules[unicode/pypinyin.py] 12.8 ms 11.3 ms +13.33%
linter/all-with-preview-rules[numpy/globals.py] 3.1 ms 3 ms +4.09%
linter/all-with-preview-rules[pydantic/types.py] 51.9 ms 47.2 ms +9.9%
linter/all-with-preview-rules[numpy/ctypeslib.py] 24.6 ms 22 ms +11.8%
parser[numpy/ctypeslib.py] 11.8 ms 10.8 ms +9.85%
linter/all-rules[large/dataset.py] 94.1 ms 82.6 ms +13.83%
linter/all-with-preview-rules[large/dataset.py] 106.9 ms 96 ms +11.39%

@charliermarsh
Copy link
Member

Lol

@AlexWaygood AlexWaygood merged commit d387d0b into astral-sh:main Feb 9, 2024
17 checks passed
@AlexWaygood AlexWaygood deleted the closing-paren branch February 9, 2024 21:27
nkxxll pushed a commit to nkxxll/ruff that referenced this pull request Mar 10, 2024
…e always on their own line (astral-sh#9793)

## Summary

Currently these rules apply the heuristic that if the original sequence
doesn't have a newline in between the final sequence item and the
closing parenthesis, the autofix won't add one for you. The feedback
from @ThiefMaster, however, was that this was producing slightly unusual
formatting -- things like this:

```py
__all__ = [
    "b", "c",
    "a", "d"]
```

were being autofixed to this:

```py
__all__ = [
    "a",
    "b",
    "c",
    "d"]
```

When, if it was _going_ to be exploded anyway, they'd prefer something
like this (with the closing parenthesis on its own line, and a trailing comma added):

```py
__all__ = [
    "a",
    "b",
    "c",
    "d",
]
```

I'm still pretty skeptical that we'll be able to please everybody here
with the formatting choices we make; _but_, on the other hand, this
_specific_ change is pretty easy to make.

## Test Plan

`cargo test`. I also ran the autofixes for RUF022 and RUF023 on CPython
to check how they looked; they looked fine to me.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants