-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Implement blank_line_after_nested_stub_class
preview style
#9155
Conversation
crates/ruff_python_formatter/tests/snapshots/format@stub_files__suite.pyi.snap
Show resolved
Hide resolved
|
@dhruvmanila what's the status of this PR? Also note that there have been some recent changes in Black. It might be worth double checking Black's tests again to ensure we match the formatting precisely. |
78028dc
to
86a5375
Compare
Here's an update regarding this preview style implementation in Ruff: tldr; there are bunch of edge cases where it seems to be difficult to add support ... What's working today:1Cases when the class definition is followed by another statement at the same indent level. This is to say that in a suite, the preceding and following value is set. Why is this easier? Because the loop takes care of a lot of conditions. For example, in the following code snippet there are 2 statements in a suite which is the body of the class Top:
class Nested1: # <- statement 1
pass
foo = 1 # <- statement 2 2Some other cases which works today is where the class is followed by a trailing comment. This is then handled by the class formatting as then it doesn't matter if there's any other statement following the class or not as we'll always need to add an empty line between the class and the comment. For example, class Top:
class Nested1:
pass
# comment This will be handled in Problematic casesNote that some of the following cases have a workaround in the PR but I'm not fully convinced with the implementation which is why they've been put in this section. The main type of case where there are problems are the ones where the class definition is the last statement in the suite. For example, 1class Top:
class Nested:
pass
foo = 1 Here, the loop will never be executed because there's just 1 statement in the suite. This means that we need to have a special case implementation for the last statement in the suite. This needs to account for a lot of other conditions to make the decision of whether to add an empty line or not: Is there a class in the parent suite which already added an empty line? If so, then we don't need to add another empty line. class Top:
class Nested11:
class Nested12:
pass
class Nested21:
pass Here, the /// Checks if the last child of the given node is a class definition without a
/// trailing comment.
pub(crate) fn is_last_child_a_class_def(node: AnyNodeRef<'_>, f: &PyFormatter) -> bool {
let comments = f.context().comments();
std::iter::successors(node.last_child_in_body(), AnyNodeRef::last_child_in_body)
.take_while(|last_child| !comments.has_trailing_own_line(*last_child))
.any(|last_child| last_child.is_stmt_class_def())
} But, this then creates another problem. Take the following code as an example: if a:
class Nested:
pass
else:
pass Here, the problem is that the new logic will add an empty line after the if a:
class Nested:
pass
else:
pass Now, the same logic will try to preserve the empty line by adding the IR element in the same position. But while formatting the 2When a comment is not trailing to the class definition but actually to one of the statements in the parent suite: if top1:
class Nested:
pass
# comment
if top2:
pass 3Similar to the previous case but at end of file: if top1:
class Nested:
pass
# comment Number of empty linesWe also need to make sure that running the formatter multiple times doesn't keep on adding empty lines (at max it could go to 2). This would happen when the new logic would add an empty line on the first run and then on the second run another logic sees the empty line which it then tries to maintain. This adds multiple empty line IR elements. Possible solutionsInstead of looking ahead, we'll always look behind which is to say that for the current node we need to query the absolute previous node. But we'll have to add the logic in multiple places - suite, if, match, try, etc. Here, absolute previous in the literal sense, not just in the same indent level. For example,
Here, the previous statement for |
30bbd6c
to
a768fc3
Compare
let empty_line_condition = if is_blank_line_after_nested_stub_class_enabled(f.context()) { | ||
self.last_node.map_or(false, |preceding| { | ||
blank_line_after_nested_stub_class_condition(preceding, None, f) | ||
}) | ||
} else { | ||
false | ||
}; | ||
|
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.
This helps when the class is at the boundary location in nested statement:
if foo:
class Nested:
pass
else:
pass
The FormatLeadingAlternateBranchComments
will be called before the else
block formatting. Similarly for other blocks like match
, case
, try .. except ..
, etc.
if (empty_line_condition | ||
&& lines_after_ignoring_end_of_line_trivia(preceding.end(), source) > 1) | ||
|| (is_blank_line_after_nested_stub_class_enabled(f.context()) | ||
&& blank_line_after_nested_stub_class_condition( | ||
preceding.into(), | ||
Some(following.into()), | ||
f, | ||
)) |
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.
It's difficult to merge the is_*_enabled && *_condition
logic with empty_line_condition
variable up above like so:
let empty_line_condition = preceding_comments.has_trailing()
|| following_comments.has_leading()
|| !stub_suite_can_omit_empty_line(preceding, following, f)
|| (is_blank_line_after_nested_stub_class_enabled(f.context())
&& blank_line_after_nested_stub_class_condition(
preceding.into(),
Some(following.into()),
f,
))
The reason being that in the top level condition the boolean check is direct but in other suite kind there's an additional check of the number of empty lines. That additional check is to only preserve the empty lines if they're present while the preview style adds an empty line conditionally. So, the condition should OR the complete check which was existing earlier to complement it.
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 think what would help here is to rename empty_line_condition
into preserve_empty_line
which is different from your check which forces empty lines.
I recommend moving the blank_line_after_nested_stub_class_condition
out of the match
and next to empty_line_condition
, and name the variable require_empty_line
I would rename blank_line_after_nested_stub_class_condition
to requires_empty_line_between_stub_file_statements
(not just preserving, enforcing an empty line).
We can then rewrite the checks in the match branches to
if requires_empty_line || preserve_empty_line {
empty_line.fmt(f)
}
which I find easier to understand.
Edit: Maybe better. Could we move the check into stub_suite_can_omit_empty_line
?
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 think what would help here is to rename
empty_line_condition
intopreserve_empty_line
which is different from your check which forces empty lines.
I'm not sure if this is 100% correct. For top-level, if the condition is true, we always make sure that there is just one empty line. It could be that there isn't any empty line in the source code which means we're adding an additional empty line. But, for the nested cases, we'd only preserve any existing empty lines and limit it to 1.
I recommend moving the
blank_line_after_nested_stub_class_condition
out of thematch
and next toempty_line_condition
, and name the variablerequire_empty_line
Thanks, I'll update the code accordingly.
I would rename
blank_line_after_nested_stub_class_condition
torequires_empty_line_between_stub_file_statements
(not just preserving, enforcing an empty line).
Yeah, I'm going with is_blank_line_required_after_nested_stub_class
.
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.
Yeah, I'm going with
is_blank_line_required_after_nested_stub_class
.
Actually, with your other comment, I'm thinking of should_insert_blank_line_after_class_in_stub_file
.
pub(crate) fn blank_line_after_nested_stub_class_condition( | ||
preceding: AnyNodeRef<'_>, | ||
following: Option<AnyNodeRef<'_>>, | ||
f: &PyFormatter, | ||
) -> bool { |
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.
This is the core condition which checks whether we need to add an empty line between the preceding and following node or not.
_ => false, | ||
} | ||
} | ||
Some(_) => !comments.has_trailing_own_line(preceding), |
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.
Class with a trailing comment is handled by empty_lines_before_trailing_comments
builder.
None => std::iter::successors( | ||
preceding.last_child_in_body(), | ||
AnyNodeRef::last_child_in_body, | ||
) | ||
.take_while(|last_child| !comments.has_trailing_own_line(*last_child)) | ||
.any(|last_child| last_child.is_stmt_class_def()), |
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.
If the preceding node isn't a class, then we check if there's a class node as the last child node of the body of nested nodes. This nesting could be arbitrarily deep.
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.
The code changes overall look good to me. I think we can make some improvements to improve readability and extend documentation a bit.
I'm very surprised that there are no changed black snapshots. Do you know why that is? Are we sure we match Black's formatting?
It would help reviewers if you add an example of the expected formatting (how is it different from today) with a short explanation of the preview style to the PR. I know it's in the linked PR, but it requires multiple steps by the reviewer: Open issue, read the summary, go back to the PR, read the summary, and finally read the code.
if (empty_line_condition | ||
&& lines_after_ignoring_end_of_line_trivia(preceding.end(), source) > 1) | ||
|| (is_blank_line_after_nested_stub_class_enabled(f.context()) | ||
&& blank_line_after_nested_stub_class_condition( | ||
preceding.into(), | ||
Some(following.into()), | ||
f, | ||
)) |
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 think what would help here is to rename empty_line_condition
into preserve_empty_line
which is different from your check which forces empty lines.
I recommend moving the blank_line_after_nested_stub_class_condition
out of the match
and next to empty_line_condition
, and name the variable require_empty_line
I would rename blank_line_after_nested_stub_class_condition
to requires_empty_line_between_stub_file_statements
(not just preserving, enforcing an empty line).
We can then rewrite the checks in the match branches to
if requires_empty_line || preserve_empty_line {
empty_line.fmt(f)
}
which I find easier to understand.
Edit: Maybe better. Could we move the check into stub_suite_can_omit_empty_line
?
#9674 should ensure that black stub files with options run as stub tests. |
@@ -85,45 +86,61 @@ pub(crate) struct FormatLeadingAlternateBranchComments<'a> { | |||
|
|||
impl Format<PyFormatContext<'_>> for FormatLeadingAlternateBranchComments<'_> { | |||
fn fmt(&self, f: &mut PyFormatter) -> FormatResult<()> { | |||
let empty_line_condition = if is_blank_line_after_nested_stub_class_enabled(f.context()) { |
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.
Can you explain why this special handling is required here but not for other cases for which stub_suite_can_omit_empty_line
returns false?
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.
This is to handle cases when a class definition is the last statement in a suite. The empty line formatting in suite is a loop which handles only for in between statements i.e., both preceding and following are present. This is required only before an alternate branch because otherwise it'll be handled by the top-level suite formatting.
3982118
to
1510693
Compare
1510693
to
8b686c0
Compare
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.
Awesome. Thanks for the added documentation
Summary
This PR implements the
blank_line_after_nested_stub_class
preview style in the formatter.The logic is divided into 3 parts:
We handle (3) with
FormatLeadingAlternateBranchComments
.Test Plan
typeshed
difffixes: #8891