Handle bracketed comments on sequence patterns#6801
Merged
charliermarsh merged 1 commit intomainfrom Aug 25, 2023
Merged
Conversation
4178cca to
b1f7091
Compare
Contributor
PR Check ResultsBenchmarkLinuxWindows |
f221e7a to
f457285
Compare
b1f7091 to
5666806
Compare
dhruvmanila
reviewed
Aug 23, 2023
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(); |
Member
There was a problem hiding this comment.
I'm not able to follow this. Can you add an example/test cases for the 2 cases here?
MichaReiser
approved these changes
Aug 23, 2023
| if source[pattern.range()].starts_with('[') { | ||
| SequenceType::List | ||
| } else if source[pattern.range()].starts_with('(') { | ||
| let Some(elt) = pattern.patterns.first() else { |
Member
There was a problem hiding this comment.
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
Member
Author
There was a problem hiding this comment.
Haha ok. I can try. It felt difficult to DRY it up without making the types much more permissive.
crates/ruff_python_formatter/src/pattern/pattern_match_sequence.rs
Outdated
Show resolved
Hide resolved
konstin
approved these changes
Aug 23, 2023
| SequenceType::List => empty_parenthesized("[", dangling, "]").fmt(f), | ||
| SequenceType::TupleNoParens => { | ||
| unreachable!("If empty, it should be either tuple or list") | ||
| SequenceType::Tuple | SequenceType::TupleNoParens => { |
Member
There was a problem hiding this comment.
How would an empty TupleNoParens look like?
f457285 to
0fef07d
Compare
5666806 to
96d92a8
Compare
5d9d616 to
161e3c5
Compare
635c173 to
728e378
Compare
96d92a8 to
26f5d4b
Compare
26f5d4b to
48d2cc9
Compare
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
4 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
This PR ensures that we handle bracketed comments on sequences, like
# commenthere: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