-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Respect fmt: skip for multiple statements on same logical line
#22119
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
Conversation
|
d80a246 to
8cfae50
Compare
8cfae50 to
43f2d41
Compare
MichaReiser
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for looking into this long standing issues.
I haven't done an in-depth review but I left some initial questions. We can also use our 1:1 to discuss some of them
|
Regarding the benchmark. I don't think using hyperfine will give you meaningful numbers. There's just too much overhead in ruff's cli (config discovery, directory walking, io) in addition to the noise of measuring a cli tool. If you want to get meaningful numbers, I suggest adding a benchmark to ruff_benchmark. Even if it's just locally |
2009b0f to
f920e49
Compare
446a376 to
09f82dd
Compare
fmt: skip for multiple statements on same linefmt: skip for multiple statements on same logical line
crates/ruff_python_formatter/tests/snapshots/format@fmt_skip__semicolons.py.snap
Show resolved
Hide resolved
7d306ba to
101df8f
Compare
MichaReiser
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, this looks pretty simple now.
Do we still need the is_suppressed logic in the statement formatting?
| SimpleTokenKind::Continuation => { | ||
| tokenizer.next(); | ||
| } | ||
| SimpleTokenKind::Newline => { | ||
| return true; | ||
| } | ||
| _ => {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I always feel a bit uneasy about tokenizer calls where we have a blanket match arm that eats over any tokens.
I would write this as
while let Some(token) = tokenizer.next() {
match token {
SimpleTokenKind::Continuation => {
// Skip over the newline
tokenizer.next();
}
SimpleTokenKind::Newline => {
return true;
}
kind => {
if !kind.is_trivia() {
return false;
}
}
}
}We could even consider making the kind.is_trivia a debug assertion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add this but observe that, since we are specifically in a range between statements, I don't think any non-trivia tokens can actually occur.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ended up keeping this as is but adding a comment with the justification I gave above - let me know if you feel strongly and I'll commit your suggestion!
crates/ruff_python_formatter/resources/test/fixtures/ruff/fmt_skip/semicolons.py
Show resolved
Hide resolved
crates/ruff_python_formatter/tests/snapshots/format@range_formatting__fmt_skip.py.snap
Show resolved
Hide resolved
I am updating these because we didn't have test coverage for the different handling of `fmt: skip` comments applied to multiple statements on the same line. This is in preparation for #22119 (to show before/after deviations). Follows the same procedure as in #20794 Edit: As it happens, the new fixtures do not even cover the case relevant to #22119 - they just deal with the already handled case of a one-line compound statement. Nevertheless, it seems worthwhile to make this update, especially since it uncovered a (possible?) bug.
101df8f to
459eca8
Compare
|
While fixing the behavior for range formatting, I noticed that, unrelated to any suppression comments, we do the following: x=1;<RANGE_START>x=2<RANGE_END>becomes: x=1; x = 2Is this expected or should I make an issue for it? The enclosing node of the range here is correctly determined to be the full suite (because it can't find the indentation for x=1
x=2back down. My guess is that this comes down to the |
|
As you predicted, removing (except for a teensy one on |
Yeah, this looks very wrong. Opening an issue for it would be great |
|
Thanks a lot for your work on this @dylwil3, and everyone who was also involved! ♥ |
This PR adjusts the logic for skipping formatting so that a
fmt: skipcan affect multiple statements if they lie on the same line.Specifically, a
fmt: skipcomment will now suppress all the statements in the suite in which it appears whose range intersects the line containing the skip directive. For example:remains unchanged after formatting.
(Note that compound statements are somewhat special and were handled in a previous PR - see #20633).
Closes #17331 and #11430.
Simplest to review commit by commit - the key diffs of interest are the commit introducing the core logic, and the diff between the snapshots introduced in the last commit (compared to the second commit).
Implementation
On
mainwe format a suite of statements by iterating through them. If we meet a statement with a leading or trailing (own-line)fmt: offcomment, then we suppress formatting until we meet afmt: oncomment. Otherwise we format the statement using its own formatting rule.How are
fmt: skipcomments handled then? They are handled internally to the formatting of each statement. Specifically, calling.fmton a statement node will first check to see if there is a trailing, end-of-linefmt: skip(orfmt: off/yapf: off), and if so then write the node with suppressed formatting.In this PR we move the responsibility for handling
fmt: skipinto the formatting logic of the suite itself. This is done as follows:Addendum on range formatting
We also had to make some changes to range formatting in order to support this new behavior. For example, we want to make sure that
formats verbatim, rather than becoming
Recall that range formatting proceeds in two steps:
We had to modify (1), since the suite knows how to format skipped nodes, but nodes may not "know" they are skipped. To do this we altered the
visit_bodybethod of theFindEnclosingNodevisitor: now we iterate through the statements and check for skipped ranges intersecting the format range. If we find them, we return without descending. The result is to consider the statement containing the suite as the enclosing node in this case.