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 should add separators for control flow #1364

Closed
ColinKennedy opened this issue Apr 25, 2020 · 2 comments
Closed

Black should add separators for control flow #1364

ColinKennedy opened this issue Apr 25, 2020 · 2 comments
Labels
T: enhancement New feature or request

Comments

@ColinKennedy
Copy link

When Python statements aren't split with whitespace, relevant details like "which block is this else meant for" get lost.

def do_something():
    if False:
        foo()
    elif True:
        do()
    for _ in range(10):
        break
    else:
        fizz()
    try:
        print("Try something")
    except:
        print("Exception!")
    else:
        fizz()
    if True:
        buzz()

There are 2 else blocks, neither of which apply to the nearby if blocks. But you may not notice because of the layout.

Compare that to:

def do_something():
    if False:
        foo()
    elif True:
        do()

    for _ in range(10):
        break
    else:
        fizz()

    try:
        print("Try something")
    except:
        print("Exception!")
    else:
        fizz()

    if True:
        buzz()

Now it's clear where one block starts and another ends. At the moment I'm accomplishing this by running black as a first pass and then using parso to add newlines for each found block. The general rule of thumb I take take is

"if it's not the first source code line, it needs 1 newline before the block". So you won't end up with situations like

for _ in range(10):

    if True:
        pass

The same logic applies to control flow statements. break, continue, return, and yield.

Code like this is hard to decipher.

def do_something():
    with Context():
        for _ in range(10):
            for index in range(10):
                if index == 1:
                    continue
            break
            try:
                foo()
            finally:
                bar()
        return afar()

The break and return statements in this scenario have incorrect indents. break should be in the inner for loop and return should be outside of the with block. It's hard to tell that they're on incorrect indents because nothing separates the blocks.

def do_something():
    with Context():
        for _ in range(10):
            for index in range(10):
                if index == 1:
                    continue

            break

            try:
                foo()
            finally:
                bar()

        return afar()

Now that the statements are separated, it's clear that break shouldn't be where it is. And because the return line isn't competing with bar(), it's easier to read, too.

I'd love to see this be brought to black because it's been so helpful.

@ColinKennedy ColinKennedy added the T: enhancement New feature or request label Apr 25, 2020
@ichard26
Copy link
Collaborator

ichard26 commented Apr 28, 2020

These changes were already addressed in #1219 and #90. I would recommend reading up on them (TL:DR is below though). Quoting from ambv in #1219:

Black used to initially have a strong opinion about all blank lines. My initial thinking was that if you need to split your functions into sections... you want more functions. But this argument proved to be extremely unpopular so I backed out of it.

And ftruzzi, if you look at the history of the project, Black initially did put blank lines after control flow statements (return, raise, continue, and break). I like this myself but unfortunately there were annoying edge cases with it so it was not a good rule to enforce every time.

So in general, I think we won't be doing what this request is suggesting. There are related bugs (like handling double blank lines in functions that are preceeded by a comment line; or standardizing on no blank lines after docstrings) that we might address. But we sadly cannot enforce any blanket usage of blank lines.

TL:DR is that changes of this kind are a bit too controversial and would've hurt the adoption of Black, so it was decided to not enforce any form of usage of single empty lines within functions. It also wouldn't help that this feature would be have a few annoying edge cases. Therefore you as the user will have to add single empty lines after control flow statements.

@cooperlees
Copy link
Collaborator

Previously discussed and controversial. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T: enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants