RUF022, RUF023: Ensure closing parentheses for multiline sequences are always on their own line#9793
Merged
AlexWaygood merged 4 commits intoastral-sh:mainfrom Feb 9, 2024
Merged
Conversation
…e always on their own line
Contributor
|
AlexWaygood
commented
Feb 2, 2024
fd85029 to
eb19a92
Compare
| // 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(); |
Member
There was a problem hiding this comment.
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.
Member
Author
There was a problem hiding this comment.
Yes, definitely doable -- just slightly less elegant here :)
I'll make the change!
Member
There was a problem hiding this comment.
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.
CodSpeed Performance ReportMerging #9793 will improve performances by 44.39%Comparing Summary
Benchmarks breakdown
|
Member
|
Lol |
charliermarsh
approved these changes
Feb 9, 2024
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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:
were being autofixed to this:
When, if it was going to be exploded anyway, they'd prefer something like this (with the closing parenthesis on its own line):
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.