Skip to content

Commit

Permalink
Review changes
Browse files Browse the repository at this point in the history
  • Loading branch information
@dhruvmanila
dhruvmanila committed Jan 30, 2024
1 parent 42770cb commit 1510693
Show file tree
Hide file tree
Showing 4 changed files with 154 additions and 83 deletions.
94 changes: 47 additions & 47 deletions crates/ruff_python_formatter/src/comments/format.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use crate::comments::{CommentLinePosition, SourceComment};
use crate::context::NodeLevel;
use crate::prelude::*;
use crate::preview::is_blank_line_after_nested_stub_class_enabled;
use crate::statement::suite::blank_line_after_nested_stub_class_condition;
use crate::statement::suite::should_insert_blank_line_after_class_in_stub_file;

/// Formats the leading comments of a node.
pub(crate) fn leading_node_comments<T>(node: &T) -> FormatLeadingComments
Expand Down Expand Up @@ -86,61 +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()) {
let require_empty_line = if is_blank_line_after_nested_stub_class_enabled(f.context())
&& f.options().source_type().is_stub()
{
self.last_node.map_or(false, |preceding| {
blank_line_after_nested_stub_class_condition(preceding, None, f)
should_insert_blank_line_after_class_in_stub_file(
preceding,
None,
f.context().comments(),
)
})
} else {
false
};

if let Some(first_leading) = self.comments.first() {
if empty_line_condition {
write!(f, [empty_line()])?;
} else {
// Leading comments only preserves the lines after the comment but not before.
// Insert the necessary lines.
write!(
f,
[empty_lines(lines_before(
first_leading.start(),
f.context().source()
))]
)?;
}
if require_empty_line {
write!(f, [empty_line(), leading_comments(self.comments)])?;
} else if let Some(first_leading) = self.comments.first() {
// Leading comments only preserves the lines after the comment but not before.
// Insert the necessary lines.
write!(
f,
[empty_lines(lines_before(
first_leading.start(),
f.context().source()
))]
)?;

write!(f, [leading_comments(self.comments)])?;
} else if let Some(last_preceding) = self.last_node {
if empty_line_condition {
write!(f, [empty_line()])?;
} else {
// The leading comments formatting ensures that it preserves the right amount of lines
// after We need to take care of this ourselves, if there's no leading `else` comment.
// Since the `last_node` could be a compound node, we need to skip _all_ trivia.
//
// For example, here, when formatting the `if` statement, the `last_node` (the `while`)
// would end at the end of `pass`, but we want to skip _all_ comments:
// ```python
// if True:
// while True:
// pass
// # comment
//
// # comment
// else:
// ...
// ```
//
// `lines_after_ignoring_trivia` is safe here, as we _know_ that the `else` doesn't
// have any leading comments.
write!(
f,
[empty_lines(lines_after_ignoring_trivia(
last_preceding.end(),
f.context().source()
))]
)?;
}
// The leading comments formatting ensures that it preserves the right amount of lines
// after We need to take care of this ourselves, if there's no leading `else` comment.
// Since the `last_node` could be a compound node, we need to skip _all_ trivia.
//
// For example, here, when formatting the `if` statement, the `last_node` (the `while`)
// would end at the end of `pass`, but we want to skip _all_ comments:
// ```python
// if True:
// while True:
// pass
// # comment
//
// # comment
// else:
// ...
// ```
//
// `lines_after_ignoring_trivia` is safe here, as we _know_ that the `else` doesn't
// have any leading comments.
write!(
f,
[empty_lines(lines_after_ignoring_trivia(
last_preceding.end(),
f.context().source()
))]
)?;
}

Ok(())
Expand Down
141 changes: 105 additions & 36 deletions crates/ruff_python_formatter/src/statement/suite.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -470,16 +470,15 @@ fn stub_file_empty_lines(
let empty_line_condition = preceding_comments.has_trailing()
|| following_comments.has_leading()
|| !stub_suite_can_omit_empty_line(preceding, following, f);
let require_empty_line = is_blank_line_after_nested_stub_class_enabled(f.context())
&& should_insert_blank_line_after_class_in_stub_file(
preceding.into(),
Some(following.into()),
f.context().comments(),
);
match kind {
SuiteKind::TopLevel => {
if empty_line_condition
|| (is_blank_line_after_nested_stub_class_enabled(f.context())
&& blank_line_after_nested_stub_class_condition(
preceding.into(),
Some(following.into()),
f,
))
{
if empty_line_condition || require_empty_line {
empty_line().fmt(f)
} else {
hard_line_break().fmt(f)
Expand All @@ -488,12 +487,7 @@ fn stub_file_empty_lines(
SuiteKind::Class | SuiteKind::Other | SuiteKind::Function => {
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,
))
|| require_empty_line
{
empty_line().fmt(f)
} else {
Expand All @@ -505,35 +499,110 @@ fn stub_file_empty_lines(

/// Checks if an empty line should be inserted between the preceding and, optionally,
/// the following node according to the [`blank_line_after_nested_stub_class`](https://github.com/astral-sh/ruff/issues/8891)
/// preview style.
/// preview style rules.
///
/// If `following` is `None`, then the preceding node is the last node in the suite.
pub(crate) fn blank_line_after_nested_stub_class_condition(
/// If `following` is `None`, then the preceding node is the last one in a suite. The
/// caller needs to make sure that the suite which the preceding node is part of is
/// followed by an alternate branch.
pub(crate) fn should_insert_blank_line_after_class_in_stub_file(
preceding: AnyNodeRef<'_>,
following: Option<AnyNodeRef<'_>>,
f: &PyFormatter,
comments: &Comments<'_>,
) -> bool {
let comments = f.context().comments();
match preceding.as_stmt_class_def() {
Some(class) if contains_only_an_ellipsis(&class.body, comments) => {
!class.decorator_list.is_empty()
|| match following {
Some(AnyNodeRef::StmtClassDef(ast::StmtClassDef {
body,
decorator_list,
..
})) => !contains_only_an_ellipsis(body, comments) || !decorator_list.is_empty(),
Some(AnyNodeRef::StmtFunctionDef(_)) | None => true,
_ => false,
}
let Some(following) = following else {
// The formatter is at the start of an alternate branch such as
// an `else` block.
//
// ```python
// if foo:
// class Nested:
// pass
// else:
// pass
// ```
//
// In the above code, the preceding node is the `Nested` class
// which has no following node.
return true;
};

// If the preceding class has decorators, then we need to add an empty
// line even if it only contains ellipsis.
//
// ```python
// class Top:
// @decorator
// class Nested1: ...
// foo = 1
// ```
let preceding_has_decorators = !class.decorator_list.is_empty();

// If the following statement is a class definition, then an empty line
// should be inserted if it (1) doesn't just contain ellipsis, or (2) has decorators.
//
// ```python
// class Top:
// class Nested1: ...
// class Nested2:
// pass
//
// class Top:
// class Nested1: ...
// @decorator
// class Nested2: ...
// ```
//
// Both of the above examples should add a blank line in between.
let following_is_class_without_only_ellipsis_or_has_decorators =
following.as_stmt_class_def().is_some_and(|following| {
!contains_only_an_ellipsis(&following.body, comments)
|| !following.decorator_list.is_empty()
});

preceding_has_decorators
|| following_is_class_without_only_ellipsis_or_has_decorators
|| following.is_stmt_function_def()
}
Some(_) => {
// Preceding statement is a class definition whose body isn't only an ellipsis.
// Here, we should only add a blank line if the class doesn't have a trailing
// own line comment as that's handled by the class formatting itself.
!comments.has_trailing_own_line(preceding)
}
None => {
// If preceding isn't a class definition, let's check if the last statement
// in the body, going all the way down, is a class definition.
//
// ```python
// if foo:
// if bar:
// class Nested:
// pass
// if other:
// pass
// ```
//
// But, if it contained a trailing own line comment, then it's handled
// by the class formatting itself.
//
// ```python
// if foo:
// if bar:
// class Nested:
// pass
// # comment
// if other:
// pass
// ```
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())
}
Some(_) => !comments.has_trailing_own_line(preceding),
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()),
}
}

Expand Down
1 change: 1 addition & 0 deletions ..._formatter/tests/snapshots/format@stub_files__blank_line_after_nested_stub_class.pyi.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,7 @@ docstring-code = Disabled
docstring-code-line-width = "dynamic"
preview = Enabled
target_version = Py38
source_type = Stub
```

```python
Expand Down
1 change: 1 addition & 0 deletions ...matter/tests/snapshots/format@stub_files__blank_line_after_nested_stub_class_eof.pyi.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ docstring-code = Disabled
docstring-code-line-width = "dynamic"
preview = Enabled
target_version = Py38
source_type = Stub
```

```python
Expand Down

0 comments on commit 1510693

Please sign in to comment.