Skip to content
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

Insert newline after nested function or class statements #7946

Merged

Conversation

konstin
Copy link
Member

@konstin konstin commented Oct 13, 2023

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
Copy link
Member Author

konstin commented Oct 13, 2023

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@konstin

This comment was marked as outdated.

@konstin 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
@konstin 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 konstin changed the base branch from Remove_empty_line_before_raw_dostrings to main October 17, 2023 13:21
@konstin konstin marked this pull request as ready for review October 17, 2023 13:27
@konstin konstin added the formatter Related to the formatter label Oct 17, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Oct 17, 2023

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines 201 to 208
.filter(|last_child| !comments.has_trailing_own_line(*last_child))
})
.any(|last_child| {
matches!(
last_child,
AnyNodeRef::StmtFunctionDef(_) | AnyNodeRef::StmtClassDef(_)
)
})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

};

body.last().map(AnyNodeRef::from)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What was the motivation for making this an associated function?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Not necessarily disagreeing, just looking to understand.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@konstin konstin enabled auto-merge (squash) October 18, 2023 09:35
@konstin konstin merged commit 0c3123e into main Oct 18, 2023
15 checks passed
@konstin konstin deleted the 10-13-Insert_newline_after_nested_function_or_class_statements branch October 18, 2023 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
formatter Related to the formatter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants