Insert newline after nested function or class statements by konstin · Pull Request #7946 · astral-sh/ruff

konstin · 2023-10-13T15:01:51Z

Summary Insert a newline after nested function and class definitions, unless there is a trailing own line comment.

We need to e.g. format

if platform.system() == "Linux":
    if sys.version > (3, 10):
        def f():
            print("old")
    else:
        def f():
            print("new")
    f()

as

if platform.system() == "Linux":
    if sys.version > (3, 10):

        def f():
            print("old")

    else:

        def f():
            print("new")

    f()

even though f() is directly preceded by an if statement, not a function or class definition. See the comments and fixtures for trailing own line comment handling.

Test Plan I checked that the new content of newlines.py matches black's formatting.

konstin · 2023-10-13T15:02:05Z

Current dependencies on/for this PR:

main
- PR Insert newline after nested function or class statements #7946 👈
  - PR Move {AnyNodeRef, AstNode} to ruff_python_ast crate root #8011

This comment was auto-generated by Graphite.

github-actions · 2023-10-17T13:37:43Z

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

MichaReiser

Leaving to @charliermarsh for the final review as his more familiar with the empty line rules than I

MichaReiser · 2023-10-17T23:23:52Z

crates/ruff_python_formatter/src/statement/suite.rs

+                        .filter(|last_child| !comments.has_trailing_own_line(*last_child))
+                })
+                .any(|last_child| {
+                    matches!(
+                        last_child,
+                        AnyNodeRef::StmtFunctionDef(_) | AnyNodeRef::StmtClassDef(_)
+                    )
+                })


Is any here correct? Shouldn't we only test this for the last node:

if True: if False: def double(s): ... #trailing comment, if gets excluded by the `filter` and the `any` will be true.

I think successors will short circuit as soon as it sees a node with a trailing comment, so the any would be false.

But I may be misunderstanding, it's a bit confusing.

Tried to make the code clearer, it's a bit complex with the recursive logic unfortunately

A quality of life improvement mostly mechanical refactoring.

…r_class_statements

charliermarsh · 2023-10-18T02:14:10Z

crates/ruff_python_formatter/src/comments/placement.rs

-    };
-
-    body.last().map(AnyNodeRef::from)
-}


What was the motivation for making this an associated function?

(Not necessarily disagreeing, just looking to understand.)

I needed to share it outside of placement where it used to be private helper function and then it seemed natural that "what is the last child of this node" is a property of node

crates/ruff_python_formatter/resources/test/fixtures/ruff/newlines.py

crates/ruff_python_formatter/src/statement/suite.rs

konstin mentioned this pull request Oct 13, 2023

Remove empty line before raw dostrings #7944

Closed

konstin mentioned this pull request Oct 13, 2023

Insert two empty lines after top level suite with trailing def/class #7947

Closed

konstin marked this pull request as draft October 13, 2023 15:36

This comment was marked as outdated.

Sign in to view

konstin force-pushed the 10-13-Insert_newline_after_nested_function_or_class_statements branch 2 times, most recently from 35c6120 to 9d6354c Compare October 17, 2023 12:36

Insert newline after nested function or class statements

f4978c6

konstin force-pushed the 10-13-Insert_newline_after_nested_function_or_class_statements branch from 9d6354c to f4978c6 Compare October 17, 2023 13:21

konstin changed the base branch from Remove_empty_line_before_raw_dostrings to main October 17, 2023 13:21

konstin marked this pull request as ready for review October 17, 2023 13:27

konstin added the formatter Related to the formatter label Oct 17, 2023

konstin mentioned this pull request Oct 17, 2023

Move {AnyNodeRef, AstNode} to ruff_python_ast crate root #8011

Merged

MichaReiser requested a review from charliermarsh October 17, 2023 23:17

MichaReiser reviewed Oct 17, 2023

View reviewed changes

konstin and others added 2 commits October 17, 2023 19:49

Move {AnyNodeRef, AstNode} to ruff_python_ast crate root (#8011)

5117a41

A quality of life improvement mostly mechanical refactoring.

Merge branch 'main' into 10-13-Insert_newline_after_nested_function_o…

677c05d

…r_class_statements

charliermarsh reviewed Oct 18, 2023

View reviewed changes

crates/ruff_python_formatter/resources/test/fixtures/ruff/newlines.py Show resolved Hide resolved

charliermarsh reviewed Oct 18, 2023

View reviewed changes

crates/ruff_python_formatter/src/statement/suite.rs Outdated Show resolved Hide resolved

charliermarsh reviewed Oct 18, 2023

View reviewed changes

crates/ruff_python_formatter/src/statement/suite.rs Outdated Show resolved Hide resolved

charliermarsh approved these changes Oct 18, 2023

View reviewed changes

konstin added 3 commits October 18, 2023 11:23

Make the comment clearer

728bce5

Make another comment clearer

ddaaba6

Update fixture comment

535f354

konstin enabled auto-merge (squash) October 18, 2023 09:35

konstin merged commit 0c3123e into main Oct 18, 2023

konstin deleted the 10-13-Insert_newline_after_nested_function_or_class_statements branch October 18, 2023 09:45

p-linnane mentioned this pull request Oct 19, 2023

ruff 0.1.1 Homebrew/homebrew-core#151871

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Insert newline after nested function or class statements#7946

Insert newline after nested function or class statements#7946
konstin merged 6 commits intomainfrom
10-13-Insert_newline_after_nested_function_or_class_statements

konstin commented Oct 13, 2023 •

edited

Loading

Uh oh!

konstin commented Oct 13, 2023 •

edited

Loading

Uh oh!

This comment was marked as outdated.

github-actions bot commented Oct 17, 2023 •

edited

Loading

Uh oh!

MichaReiser left a comment

Uh oh!

MichaReiser Oct 17, 2023

Uh oh!

charliermarsh Oct 18, 2023

Uh oh!

charliermarsh Oct 18, 2023

Uh oh!

konstin Oct 18, 2023

Uh oh!

charliermarsh Oct 18, 2023

Uh oh!

charliermarsh Oct 18, 2023

Uh oh!

konstin Oct 18, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

konstin commented Oct 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

konstin commented Oct 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

github-actions bot commented Oct 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Check Results

Ecosystem

Uh oh!

MichaReiser left a comment

Choose a reason for hiding this comment

Uh oh!

MichaReiser Oct 17, 2023

Choose a reason for hiding this comment

Uh oh!

charliermarsh Oct 18, 2023

Choose a reason for hiding this comment

Uh oh!

charliermarsh Oct 18, 2023

Choose a reason for hiding this comment

Uh oh!

konstin Oct 18, 2023

Choose a reason for hiding this comment

Uh oh!

charliermarsh Oct 18, 2023

Choose a reason for hiding this comment

Uh oh!

charliermarsh Oct 18, 2023

Choose a reason for hiding this comment

Uh oh!

konstin Oct 18, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

konstin commented Oct 13, 2023 •

edited

Loading

konstin commented Oct 13, 2023 •

edited

Loading

github-actions bot commented Oct 17, 2023 •

edited

Loading