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.

Sign up for GitHub

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

Jump to bottom

Handle bracketed comments on sequence patterns #6801

Merged
merged 1 commit into from
Aug 25, 2023
Merged

Handle bracketed comments on sequence patterns #6801

merged 1 commit into from
Aug 25, 2023

Conversation

charliermarsh
Copy link
Member

Summary

This PR ensures that we handle bracketed comments on sequences, like # comment here:

match x:
    case [ # comment
        1, 2
    ]:
        pass

The handling is very similar to other, similar nodes, except that we do need some special logic to determine whether the sequence is parenthesized, similar to our logic for tuples.

Test Plan

cargo test

@charliermarsh charliermarsh added the formatter Related to the formatter label Aug 23, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Aug 23, 2023 •
edited
Loading

PR Check Results

Benchmark

Linux

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00      4.1±0.02ms    10.0 MB/sec    1.00      4.1±0.02ms    10.0 MB/sec
formatter/numpy/ctypeslib.py               1.00    848.9±3.25µs    19.6 MB/sec    1.00    850.4±2.93µs    19.6 MB/sec
formatter/numpy/globals.py                 1.00     78.9±0.42µs    37.4 MB/sec    1.04     81.7±0.37µs    36.1 MB/sec
formatter/pydantic/types.py                1.00   1651.5±2.53µs    15.4 MB/sec    1.00   1653.5±8.27µs    15.4 MB/sec
linter/all-rules/large/dataset.py          1.00     10.1±0.05ms     4.0 MB/sec    1.00     10.1±0.02ms     4.0 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      2.7±0.01ms     6.1 MB/sec    1.01      2.8±0.03ms     6.0 MB/sec
linter/all-rules/numpy/globals.py          1.00    386.1±0.84µs     7.6 MB/sec    1.00    387.8±1.11µs     7.6 MB/sec
linter/all-rules/pydantic/types.py         1.00      5.3±0.05ms     4.8 MB/sec    1.00      5.3±0.02ms     4.8 MB/sec
linter/default-rules/large/dataset.py      1.00      5.4±0.02ms     7.5 MB/sec    1.00      5.4±0.01ms     7.5 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00   1205.3±4.44µs    13.8 MB/sec    1.00   1206.0±9.55µs    13.8 MB/sec
linter/default-rules/numpy/globals.py      1.00    140.2±1.19µs    21.0 MB/sec    1.00    140.3±0.61µs    21.0 MB/sec
linter/default-rules/pydantic/types.py     1.00      2.5±0.03ms    10.3 MB/sec    1.00      2.5±0.04ms    10.3 MB/sec

Windows

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00      5.2±0.38ms     7.8 MB/sec    1.00      5.2±0.11ms     7.8 MB/sec
formatter/numpy/ctypeslib.py               1.00   999.5±64.21µs    16.7 MB/sec    1.07  1065.5±50.11µs    15.6 MB/sec
formatter/numpy/globals.py                 1.00     88.8±3.18µs    33.2 MB/sec    1.09     97.0±8.32µs    30.4 MB/sec
formatter/pydantic/types.py                1.00  1938.4±92.54µs    13.2 MB/sec    1.09      2.1±0.09ms    12.0 MB/sec
linter/all-rules/large/dataset.py          1.24     15.4±1.08ms     2.6 MB/sec    1.00     12.4±0.38ms     3.3 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.25      4.4±0.31ms     3.8 MB/sec    1.00      3.5±0.12ms     4.7 MB/sec
linter/all-rules/numpy/globals.py          1.19   522.9±24.13µs     5.6 MB/sec    1.00   441.0±20.86µs     6.7 MB/sec
linter/all-rules/pydantic/types.py         1.22      8.2±0.60ms     3.1 MB/sec    1.00      6.7±0.25ms     3.8 MB/sec
linter/default-rules/large/dataset.py      1.18      8.5±0.47ms     4.8 MB/sec    1.00      7.2±0.51ms     5.6 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.36      2.1±0.18ms     8.0 MB/sec    1.00  1517.5±64.48µs    11.0 MB/sec
linter/default-rules/numpy/globals.py      1.08   224.8±11.07µs    13.1 MB/sec    1.00   207.5±14.47µs    14.2 MB/sec
linter/default-rules/pydantic/types.py     1.20      4.0±0.19ms     6.5 MB/sec    1.00      3.3±0.31ms     7.7 MB/sec

dhruvmanila
dhruvmanila reviewed Aug 23, 2023
View reviewed changes
crates/ruff_python_formatter/src/pattern/pattern_match_sequence.rs Outdated Show resolved Hide resolved
crates/ruff_python_formatter/src/pattern/pattern_match_sequence.rs
Comment on lines +74 to +109
let close_parentheses_count =
SimpleTokenizer::new(source, TextRange::new(elt.end(), elt.end()))
.skip_trivia()
.take_while(|token| token.kind() != SimpleTokenKind::Comma)
.filter(|token| token.kind() == SimpleTokenKind::RParen)
.count();
Copy link
Member

Choose a reason for hiding this comment

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

I'm not able to follow this. Can you add an example/test cases for the 2 cases here?

MichaReiser
MichaReiser approved these changes Aug 23, 2023
View reviewed changes
crates/ruff_python_formatter/src/pattern/pattern_match_sequence.rs
if source[pattern.range()].starts_with('[') {
SequenceType::List
} else if source[pattern.range()].starts_with('(') {
let Some(elt) = pattern.patterns.first() else {
Copy link
Member

Choose a reason for hiding this comment

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

This seems to be the same as is_tuple_parenthesized. It may make sense to make the code reusable (as it is somewhat tricky). It would also be helpful to add an example of why the complex open > close paren path is necessary. I already forgot and had to look it up on the PR 95f7882

Copy link
Member Author

Choose a reason for hiding this comment

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

Haha ok. I can try. It felt difficult to DRY it up without making the types much more permissive.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, don't try too hard.

crates/ruff_python_formatter/src/pattern/pattern_match_sequence.rs Outdated Show resolved Hide resolved
konstin
konstin approved these changes Aug 23, 2023
View reviewed changes
crates/ruff_python_formatter/src/pattern/pattern_match_sequence.rs
SequenceType::List => empty_parenthesized("[", dangling, "]").fmt(f),
SequenceType::TupleNoParens => {
unreachable!("If empty, it should be either tuple or list")
SequenceType::Tuple | SequenceType::TupleNoParens => {
Copy link
Member

Choose a reason for hiding this comment

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

How would an empty TupleNoParens look like?

@charliermarsh charliermarsh force-pushed the charlie/paren-value branch 2 times, most recently from 5d9d616 to 161e3c5 Compare August 23, 2023 15:37
@charliermarsh charliermarsh mentioned this pull request Aug 23, 2023
Merged
@charliermarsh charliermarsh force-pushed the charlie/paren-value branch 2 times, most recently from 635c173 to 728e378 Compare August 25, 2023 03:38
Base automatically changed from charlie/paren-value to main August 25, 2023 03:45
@charliermarsh charliermarsh enabled auto-merge (squash) August 25, 2023 03:54
@charliermarsh charliermarsh merged commit f754ad5 into main Aug 25, 2023
@charliermarsh charliermarsh deleted the charlie/bracketed branch August 25, 2023 04:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dhruvmanila dhruvmanila dhruvmanila left review comments

@MichaReiser MichaReiser MichaReiser approved these changes

@konstin konstin konstin approved these changes

Assignees
No one assigned
Labels
formatter Related to the formatter
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@charliermarsh @MichaReiser @konstin @dhruvmanila