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 enforce zero empty leading lines in function body #902

Closed
rogalski opened this issue Jun 23, 2019 · 12 comments · Fixed by #3035
Closed

Black should enforce zero empty leading lines in function body #902

rogalski opened this issue Jun 23, 2019 · 12 comments · Fixed by #3035
Labels
F: empty lines Wasting vertical space efficiently. S: accepted The changes in this design / enhancement issue have been accepted and can be implemented T: style What do we want Blackened code to look like?

Comments

@rogalski
Copy link

Operating system: playground
Python version: playground
Black version: master

Consider following code snippet:

class X:
    
    
    def f(self, x):
        
        
        return x**2

When class definition is formatted, empty lines between class X: line and first node of the body is forced to be zero.
When function definition is formatted, empty lines between def f(): line and first node of the body is normalised to either 0 (if input code had 0 empty lines) or 1 (if input code had 1+ empty lines).

While it's preferable to preserve some of the empty lines of input code, doing this for leading empty lines of node body makes no sense, as this empty line cannot have any mental meaning in such context.

@zsol zsol added T: style What do we want Blackened code to look like? F: empty lines Wasting vertical space efficiently. labels Jul 30, 2019
@felix-hilden
Copy link
Collaborator

I would assume this should also apply to classes, and I'd go as far as to say that all blocks should have their leading vertical whitespace removed, for example:

if a:

    b

But they could surely be taken care of in separate issues. More like a mental note for now.

@tolomea
Copy link

tolomea commented Mar 1, 2022

That sounds right I can't think of any reason to have blank lines around an increase in indent level, the increase it self is quite visually clear.
It's a bit different with dedents which often end logical blocks.

@ichard26 ichard26 added the S: accepted The changes in this design / enhancement issue have been accepted and can be implemented label May 18, 2022
@jenstroeger
Copy link

jenstroeger commented Feb 10, 2023

I’m a bit late to the discussion, but I still wanted to chime in after a few empty lines were removed with which I quite disagree. In general, yes the above makes sense for one exception: comments.

If I start a code block with a comment then I always have an empty line before; it makes reading easier:

def some_function():
    """Does something nifty."""
    
    # And here we go with the first code block.
    if condition:

        # And more explanation here because this next code block is funky.
        foo = ...

With merging PR #3035, however, this is all being squashed and mushed and compacted together which makes the code unreadable. It also defeats the purpose of comments separating code blocks:

def some_function():
    """Does something nifty."""
    # And here we go with the first code block.
    if condition:
        # And more explanation here because this next code block is funky.
        foo = ...

Any chance this all can be loosened up some?

@bluenote10
Copy link

The original post is slightly misleading, because multiple empty lines at the beginning of a block is certainly odd, and something black has prevented for a long time already. See #3551 for some "objective" (as far as one can get) arguments why allowing either zero or one empty line can have advantages from a typographic / design perspective.

@jenstroeger
Copy link

Thanks @bluenote10, I actually commented there as well and referenced my comment. Perhaps I should actually copy the comment over 🤔

@tolomea
Copy link

tolomea commented Feb 10, 2023

@bluenote10 "allowing either zero or one" is supporting optional whitespace and having optional whitespace either in the style or as command line options feels like a contradiction of the point of Black.

I liked the change here, before some of our functions had a blank line at the start and some didn't. Now they are all the same.

You could make an argument for always having a blank link between def and the first code block, but I don't think that's a majority POV.

Some of the things Black does I didn't agree with, but I adapted, that is part of the price of consistency.

@bluenote10
Copy link

bluenote10 commented Feb 10, 2023

@tolomea There are basic principles in typography that build on how the human perception works, and this rule violates these principles. For instance, there is a reason why standard typography rules lays out blocks like this

image

and not like this

image

Whitespace can reinforce perception of block structure and, thus, improve readability. It is a fundamental principle of how the human perception works, and we should not ignore that.

There is another way to see why this change is detrimental: Currently (with the old black version), we have a choice at the beginning of each block between using 0 spacing lines or 1 spacing line. With this change, there is no degree of freedom anymore, it is always 0 spacing. This would make sense if in 100% of all occurrences the majority of developers (i.e., more than 50%) would agree that the 0-space variant is more readable than the 1-space variant. This is not the case however. Clearly there will be many cases where the majority will perceive 0-space as perfectly fine (like the example in the OP). Nonetheless, there will still be cases where a clear majority, say perhaps 8 out of 10 developers, would perceive the 1-space variant as significantly more readable. From my experience this is particularly the case when both the line opening the block and the first line in the block is long and verbose. We are now seeing hundreds of such cases, where we don't want to apply the less readable newly enforced style. There is a reason why these occurrences have been written like that in the first place.

Also, in practice I've never encountered an abuse of this particular degree of freedom. The examples above feel pathological. Just because you can use an empty newline at the beginning of a block doesn't mean that developers necessarily obsess with it. If this was the case we would have a problem anyway, because they can intercept their entire code with unhelpful newlines within a code block anyway.

@JordanZimmitti
Copy link

JordanZimmitti commented Feb 10, 2023

@bluenote10 What you have above is spot on. It's important to also highlight @jenstroeger example as well which resembles a lot of my code:

def some_function():
    """Does something nifty."""
    # And here we go with the first code block.
    if condition:
        # And more explanation here because this next code block is funky.
        foo = ...

Having no new lines between comments is much harder to read

@jenstroeger
Copy link

Thank you @bluenote10 and @JordanZimmitti, I couldn’t agree more.

@naught101
Copy link

naught101 commented May 25, 2023

How does something like this get merged with almost zero rationale and zero evidence that it's actually beneficial?

Bluenote's logic also applies to blocks that are NOT control flow statements or other compound statements (as discussed at #3551 ) e.g.

if blah:

    # Do some stuff that is logically related
    data = get_data()

    # Do some different stuff that is logically related
    results = calculate_results()

    return results

is far more readable than

if blah:
    # Do some stuff that is logically related
    data = get_data()

    # Do some different stuff that is logically related
    results = calculate_results()

    return results

because the if logic is not logically related to the data-getting logic below.

Please revert.

@bmcandr
Copy link

bmcandr commented Aug 10, 2023

I agree with the dissension in this thread. At the very least, please make this rule configurable.

@vonHartz
Copy link

vonHartz commented Nov 2, 2023

I'm surprised that the only examples posted in this discussion are with zero function parameters.

In practice, function signatures are way more complex, especially with type hints.
Not only are type hints becoming pretty standard these days, but having function declarations that are easy to parse visually, is imho one of the main reasons to use Black.

Consider this minimal example. As @bluenote10 pointed out above, the block structure is much easier to infer with a newline. With it, is way clearer where the signature ends and where the body begins.

def function(
    self, data: torch.Tensor | tuple[torch.Tensor, ...], other_argument: int
) -> torch.Tensor | tuple[torch.Tensor, ...]:
    do_something(data)
    return something

def function(
    self, data: torch.Tensor | tuple[torch.Tensor, ...], other_argument: int
) -> torch.Tensor | tuple[torch.Tensor, ...]:

    do_something(data)
    return something

And it gets worse for longer signatures.

    def func(
        self,
        arg1: int,
        arg2: bool,
        arg3: bool,
        arg4: float,
        arg5: bool,
    ) -> tuple[...]:
        if arg2 and arg3:
            raise ValueError

    def func(
        self,
        arg1: int,
        arg2: bool,
        arg3: bool,
        arg4: float,
        arg5: bool,
    ) -> tuple[...]:

        if arg2 and arg3:
            raise ValueError

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F: empty lines Wasting vertical space efficiently. S: accepted The changes in this design / enhancement issue have been accepted and can be implemented T: style What do we want Blackened code to look like?
Projects
None yet
Development

Successfully merging a pull request may close this issue.