From 8b686c0586d9f6c84fe9ce1532c8730b78b1ced0 Mon Sep 17 00:00:00 2001 From: Dhruv Manilawala Date: Tue, 30 Jan 2024 19:53:23 +0530 Subject: [PATCH] Review changes --- .../src/comments/format.rs | 94 ++++++------ .../src/statement/suite.rs | 141 +++++++++++++----- ..._compatibility@cases__nested_stub.pyi.snap | 20 +-- ...lank_line_after_nested_stub_class.pyi.snap | 1 + ..._line_after_nested_stub_class_eof.pyi.snap | 1 + 5 files changed, 159 insertions(+), 98 deletions(-) diff --git a/crates/ruff_python_formatter/src/comments/format.rs b/crates/ruff_python_formatter/src/comments/format.rs index 2592f8c08d963..134ab017530d0 100644 --- a/crates/ruff_python_formatter/src/comments/format.rs +++ b/crates/ruff_python_formatter/src/comments/format.rs @@ -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(node: &T) -> FormatLeadingComments @@ -86,61 +86,61 @@ pub(crate) struct FormatLeadingAlternateBranchComments<'a> { impl Format> 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(()) diff --git a/crates/ruff_python_formatter/src/statement/suite.rs b/crates/ruff_python_formatter/src/statement/suite.rs index 1bb6e1c92b93f..ade17f2698559 100644 --- a/crates/ruff_python_formatter/src/statement/suite.rs +++ b/crates/ruff_python_formatter/src/statement/suite.rs @@ -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) @@ -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 { @@ -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>, - 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()), } } diff --git a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@cases__nested_stub.pyi.snap b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@cases__nested_stub.pyi.snap index 45761c1051684..5eed350fd7dbc 100644 --- a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@cases__nested_stub.pyi.snap +++ b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@cases__nested_stub.pyi.snap @@ -42,17 +42,15 @@ class TopLevel: ```diff --- Black +++ Ruff -@@ -3,33 +3,27 @@ +@@ -3,7 +3,6 @@ class Outer: class InnerStub: ... outer_attr_after_inner_stub: int - class Inner: inner_attr: int -- - outer_attr: int - if sys.version_info > (3, 7): +@@ -13,12 +12,10 @@ if sys.platform == "win32": assignment = 1 def function_definition(self): ... @@ -65,17 +63,6 @@ class TopLevel: def f2(self) -> str: ... class TopLevel: - class Nested1: - foo: int - def bar(self): ... -- - field = 1 - - class Nested2: - def bar(self): ... - foo: int -- - field = 1 ``` ## Ruff Output @@ -88,6 +75,7 @@ class Outer: outer_attr_after_inner_stub: int class Inner: inner_attr: int + outer_attr: int if sys.version_info > (3, 7): @@ -104,11 +92,13 @@ class TopLevel: class Nested1: foo: int def bar(self): ... + field = 1 class Nested2: def bar(self): ... foo: int + field = 1 ``` diff --git a/crates/ruff_python_formatter/tests/snapshots/format@stub_files__blank_line_after_nested_stub_class.pyi.snap b/crates/ruff_python_formatter/tests/snapshots/format@stub_files__blank_line_after_nested_stub_class.pyi.snap index 67f3160940b4f..4f570bba9ffa1 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@stub_files__blank_line_after_nested_stub_class.pyi.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@stub_files__blank_line_after_nested_stub_class.pyi.snap @@ -202,6 +202,7 @@ docstring-code = Disabled docstring-code-line-width = "dynamic" preview = Enabled target_version = Py38 +source_type = Stub ``` ```python diff --git a/crates/ruff_python_formatter/tests/snapshots/format@stub_files__blank_line_after_nested_stub_class_eof.pyi.snap b/crates/ruff_python_formatter/tests/snapshots/format@stub_files__blank_line_after_nested_stub_class_eof.pyi.snap index 3cba0307c189b..18410103151b7 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@stub_files__blank_line_after_nested_stub_class_eof.pyi.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@stub_files__blank_line_after_nested_stub_class_eof.pyi.snap @@ -36,6 +36,7 @@ docstring-code = Disabled docstring-code-line-width = "dynamic" preview = Enabled target_version = Py38 +source_type = Stub ``` ```python