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

Black is inconsistent with empty lines before an inner function after code block open, with or without leading comments #3300

Closed
yilei opened this issue Sep 29, 2022 · 8 comments
Labels
F: comments The syntactic kind. Not in the language grammar, always on our minds. Best bugs. F: empty lines Wasting vertical space efficiently. T: bug Something isn't working

Comments

@yilei
Copy link
Contributor

yilei commented Sep 29, 2022

To Reproduce

Unformatted code:

def main():
    if a:

        # Comment
        def b():
            pass

    if b:
        def c():
            pass

Running black --preview:

def main():
    if a:
        # Comment
        def b():
            pass

    if b:

        def c():
            pass

Expected behavior

Empty line should be consistent added (or removed) between the code block open and inner function:

def main():
    if a:

        # Comment
        def b():
            pass

    if b:

        def c():
            pass

Or,

def main():
    if a:
        # Comment
        def b():
            pass

    if b:
        def c():
            pass

Additional context

I did a bisect and this was caused by #3035.

Note that if the inner function doesn't have a leading comment, it won't remove the empty line:

def main():
    if a:

        def b():
            pass

        def c():
            pass

Thus I believe this is an undesired behavior change in #3035?

@yilei yilei added the T: bug Something isn't working label Sep 29, 2022
@yilei
Copy link
Contributor Author

yilei commented Sep 29, 2022

#246 is possibly related since that's also caused by a comment before the function def.

@ichard26 ichard26 added F: comments The syntactic kind. Not in the language grammar, always on our minds. Best bugs. F: empty lines Wasting vertical space efficiently. labels Sep 29, 2022
@ichard26
Copy link
Collaborator

I agree this should be fixed.

@yilei
Copy link
Contributor Author

yilei commented Sep 29, 2022

Looking at EmptyLineTracker, it is indeed the same underlying issue as #246, that the tracker isn't capable of doing this when there are preceding standalone comments. I'll take a stab at a fix.

@felix-hilden
Copy link
Collaborator

Having a look at #3035, I'd expect the newline to be removed before the first function regardless of the comment. Would you prefer otherwise?

@yilei
Copy link
Contributor Author

yilei commented Oct 10, 2022

I would actually personally prefer the newline to be removed after a block open.

I saw this is inconsistent with or without the comment, plus the current Black style says:

It will also insert proper spacing before and after function definitions. It’s one line before and after inner functions...

This is the "inner function" case.

So I'm slightly in favor of changing the preview style to always remove the newline before an inner function and immediately after a block open, with or without the leading comment (I'm strong on making the behavior consistent with or without comment). I can send a separate PR to do that once we make decision. Meanwhile I updated #3302 to keep the current behavior for this case.

@yilei yilei changed the title Black forcibly removes the empty line before an inner function with leading comments after code block open Black is inconsistent with empty lines before an inner function after code block open, with or without leading comments Oct 10, 2022
@felix-hilden
Copy link
Collaborator

I'm fully in favor of doing it, but it's fine to keep the original behavior if you'd like to 👍 I think the inner function spacing rule is secondary to the "no space in a new block" rule.

@yilei
Copy link
Contributor Author

yilei commented Oct 16, 2022

@felix-hilden Thanks. Mind approve #3302 and get it merged to fix #246? Then I'll send a follow-up PR to for this issue.

@felix-hilden
Copy link
Collaborator

Done, we'll wait for another approval since Jelle's is a bit stale. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F: comments The syntactic kind. Not in the language grammar, always on our minds. Best bugs. F: empty lines Wasting vertical space efficiently. T: bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants