Conversation
|
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
MichaReiser
force-pushed
the
fmt-on-off
branch
4 times, most recently
from
fcdea72 to
fccab61
Compare
| - 'unformatted' | ||
| + a, b = *hello | ||
| + "unformatted" | ||
| + #hey, that won't work |
| @@ -63,15 +63,15 @@ | ||
|
|
||
| something = { | ||
| # fmt: off |
There was a problem hiding this comment.
Ruff doesn't support expression level suppression comments.
| @@ -4,17 +4,10 @@ | ||
| 3, 4, | ||
| ]) | ||
| # fmt: on |
There was a problem hiding this comment.
Suppression comments between decorators are not supported.
MichaReiser
force-pushed
the
fmt-on-off
branch
from
fccab61 to
8df6726
Compare
| // ``` | ||
| write!(f, [empty_line()])?; | ||
| } | ||
| write!(f, [first.format()])?; |
There was a problem hiding this comment.
I had to move the first.format out to avoid having to handle suppression comments in each branch.
| write!(f, [empty_line(), second.format()])?; | ||
| last = second; |
There was a problem hiding this comment.
I had to move this into the loop to avoid having to handle the suppression comments on second
| let mut preceding = if comments | ||
| .leading_comments(first) | ||
| .iter() | ||
| .any(|comment| comment.is_suppression_off_comment(source)) | ||
| { | ||
| write_suppressed_statements_starting_with_leading_comment(first, &mut iter, f)? | ||
| } else if comments | ||
| .trailing_comments(first) | ||
| .iter() | ||
| .any(|comment| comment.is_suppression_off_comment(source)) | ||
| { | ||
| write_suppressed_statements_starting_with_trailing_comment(first, &mut iter, f)? | ||
| } else { | ||
| first.format().fmt(f)?; | ||
| first |
| } else if after_docstring { | ||
| // Enforce an empty line after a class docstring, e.g., these are both stable | ||
| // formatting: | ||
| // ```python | ||
| // class Test: | ||
| // """Docstring""" | ||
| // | ||
| // ... | ||
| // | ||
| // | ||
| // class Test: | ||
| // | ||
| // """Docstring""" | ||
| // | ||
| // ... | ||
| // ``` | ||
| empty_line().fmt(f)?; | ||
| after_docstring = false; |
| if comments | ||
| .leading_comments(following) | ||
| .iter() | ||
| .any(|comment| comment.is_suppression_off_comment(source)) | ||
| { | ||
| preceding = write_suppressed_statements_starting_with_leading_comment( | ||
| following, &mut iter, f, | ||
| )?; | ||
| } else if comments | ||
| .trailing_comments(following) | ||
| .iter() | ||
| .any(|comment| comment.is_suppression_off_comment(source)) | ||
| { | ||
| preceding = write_suppressed_statements_starting_with_trailing_comment( | ||
| following, &mut iter, f, | ||
| )?; | ||
| } else { | ||
| following.format().fmt(f)?; | ||
| preceding = following; |
PR Check ResultsBenchmarkLinuxWindows |
MichaReiser
force-pushed
the
fmt-on-off
branch
from
8df6726 to
194def6
Compare
MichaReiser
force-pushed
the
fmt-on-off
branch
from
194def6 to
d4b9675
Compare
MichaReiser
force-pushed
the
fmt-on-off
branch
from
d4b9675 to
a62ec0c
Compare
konstin
force-pushed
the
docstring_formatting
branch
from
ce6a1bf to
8b7ed41
Compare
| format_on_comment.start(), | ||
| )), | ||
| trailing_comments(std::slice::from_ref(format_on_comment)), | ||
| trailing_comments(formatted_comments), |
There was a problem hiding this comment.
Could it ever be the case that we're missing out on "special" formatting of some of these comments? I don't have a more precise idea of when that could happen, but just acknowledging that these are typically handled by children, and we're now intercepting and formatting them here.
There was a problem hiding this comment.
Good catch. This could be a problem if a statement overrides fmt_leading_comments or fmt_trailing_comments. I checked, and no expression or statement overrides these methods on FormatNodeRule. I went ahead and inlined the functions to not give the impression that this behavior should be customized.
Either way, I think it would be okay to format the comments with the default formatting. It may not be perfect but we're dealing with suppressed ranges anyway.
| } | ||
| } | ||
|
|
||
| struct CommentRangeIter<'a> { |
There was a problem hiding this comment.
Do you intentionally not #[derive(Debug)] here? (Asking for my own learning.)
There was a problem hiding this comment.
Mainly because I didn't need it, it's a private type, and printing iterators is always a bit awkward.
MichaReiser
force-pushed
the
fmt-on-off
branch
from
a62ec0c to
ce80dc9
Compare
konstin
force-pushed
the
docstring_formatting
branch
from
8b7ed41 to
a7dd46a
Compare
MichaReiser
force-pushed
the
fmt-on-off
branch
from
613d18e to
5622dac
Compare
|
The main reason why this PR regresses performance is because it is now necessary to look up the leading and trailing comments for every statement, and we do the same when formatting the statement. We could improve this by moving the leading/trailing comments formatting for statements into |
MichaReiser
force-pushed
the
fmt-on-off
branch
from
5622dac to
05cbaab
Compare
3 participants
Summary
Implements support for
fmt: offandfmt: onsuppression comments on statement level.This seems trivial but quickly gets complicated because you can have:
in which case we should not disable formatting (see test cases for more absurd usages of
fmt: offandfmt: oncombinations).This PR does not address:
Test Plan
I added many test files and believe that I covered all branches.
Small improvements in the similarity index:
Before
After