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

Only enforce blank line separation between functions and classes at the same level of nesting #450

Closed
njsmith opened this issue Aug 14, 2018 · 10 comments
Labels
F: empty lines Wasting vertical space efficiently. T: style What do we want Blackened code to look like?

Comments

@njsmith
Copy link

njsmith commented Aug 14, 2018

Black insists that function and class definitions should be separated by blank lines. Good rule! However, I think it's slightly overenthusiastic.

For functions at the same level of nesting, this is a good idea:

 def foo():
     pass
+
 def bar():
     pass

And it's not just between two functions, but between any start/end of a function and surrounding code:

 FOO = 1
+
 def foo():
     pass
+
 BAR = 2

But! This seems unnecessary – and dare I say, ugly and less readable – when the adjacent line of code is on a different nesting level:

    if clogged_stream_maker is not None:

        async def flipped_clogged_stream_maker():
            return reversed(await clogged_stream_maker())

    else:
        flipped_clogged_stream_maker = None

I claim that the above snippet (source would be better without any blank lines.

The same applies to classes, like, how does this blank line help anything? (source

 def test_wrap_non_iobase():
+
     class FakeFile:

         def close(self):  # pragma: no cover
             pass

The intuition here is that blank lines are separators, not headers/footers. And you only need separation between items at the same level of nesting. We don't insist on using blank lines to separate a class or def from the beginning or end of the file; similarly, we shouldn't insist on using them to separate a class or def from the beginning or end of a block.

This would also make black's handling of def and class more consistent with its handling of import. Currently black does use the rule I suggest for blank lines after import statements:

# Black insists on a blank line after the import here:
import foo

BAR = 1

# But not here:
try:
    import foo
except ImportError:
    pass

This annoys me because I have a codebase with lots of functions whose first order of business is to define a nested function:

async def foo():  # black wants to insert a blank line after this line
    def bar():
        ...

    return await run_sync_in_worker_thread(bar)

Note that making this change would not introduce churn into existing codebases, because black already preserves blank lines. It would just introduce fewer unnecessary blank lines in the future.

@zsol
Copy link
Collaborator

zsol commented Aug 15, 2018

I support this, @ambv what do you think?

@zsol zsol added T: style What do we want Blackened code to look like? F: empty lines Wasting vertical space efficiently. labels Aug 15, 2018
@ambv
Copy link
Collaborator

ambv commented Aug 17, 2018

I'm not convinced.

The same applies to classes, like, how does this blank line help anything?

Well, I remember a certain gentleman fight really hard back when Black was removing unnecessary vertical whitespace within function bodies ;-)

An inner class definitely fits the description of "logical sections within a function" that PEP 8 talks about:

Use blank lines in functions, sparingly, to indicate logical sections.

I like that Black is doing now because it makes it painfully obvious where scope boundaries are. I agree it's aesthetically unpleasant but it's consistent with module scope which is also code that runs from top to bottom, just like a function body.

I don't care much about the case when an inner def or class happens right at the beginning of a function (although it stops looking weird if you'd have a docstring!). But what I think we need to keep for readability is the blank line after an inner function or class. Again, scope boundaries.

Well, maybe except for the case when the following line has a lesser indentation level than the inner function/class. In other words, yes:

def outer():
    def inner():
        return False

    some = other * code

No:

def outer():
    try:
        def inner():
            return False
    except E as e:
        ...

@njsmith
Copy link
Author

njsmith commented Aug 17, 2018

I don't care much about the case when an inner def or class happens right at the beginning of a function (although it stops looking weird if you'd have a docstring!). But what I think we need to keep for readability is the blank line after an inner function or class. Again, scope boundaries.

Well, maybe except for the case when the following line has a lesser indentation level than the inner function/class.

... Did you just argue yourself around to agreeing with me? :-)

The proposal is exactly to keep the blank line, except in cases where the adjacent code has a lesser indentation.

Oh, I guess there is one case where that's different than what I originally said:

if ...:
    ...
def ...

According to how I originally phrased this ("different level of indentation") this would not need a blank line; according to how I phrased it now ("lesser level of indentation"), this does need a blank line. I actually meant "lesser" all along, I just forgot that this case existed so didn't realize it mattered which one I said :-). (At the bottom of a function/class this ambiguity can't happen, because only a lesser indentation can close the function/class.)

@ambv
Copy link
Collaborator

ambv commented Nov 6, 2018

(Similarly to #449, I want the rule that we'll implement to be trivially explicable to beginners.)

Opening blank line vs. closing blank line

I think it's uncontroversial to say that the opening blank line before an inner class or def could be omitted in all cases where the indentation level of the previous line was lower.

Similarly, with closing blank lines, we could intuit that the same rule applies: if the following line's indentation level is lower than the one of the entire inner class or def, skip the blank line. However, there's more to it.

Inner definition size

My intuition is that omitting blank lines for inner classes and defs is only beneficial if those inner definitions are short. If you have an inner definition (like in a decorator, say) which doesn't fit on one screen, you'd want an additional empty line to make it obvious some block of code ended (and the following return belongs to a different function!).

So this is optimizing for short definitions. But how short? I hate magic numbers.

Inner definition blank lines

This is somewhat related to the previous point. If the inner class or def itself contains blank lines, then skipping the closing blank line will just look weird. In fact, in this case having the symmetrical opening blank line is likely also desirable for clarity.

@njsmith
Copy link
Author

njsmith commented Apr 19, 2019

I don't have a strong intuition for how to format long nested definitions, and I don't feel the intuition that embedding a blank line inside a function somehow forces the function to need surrounding blank lines. I suspect this is getting far enough into weird-edge-case-of-weird-edge-case-land that probably any formatting is going to be somewhat unsatisfying.

In most cases where you can have a nested definition, doesn't black already allow users to insert optional blank lines? Maybe the rule would be: when you have a def or class adjacent to less-indented code, then a blank line is optional; otherwise it's mandatory.

@ambv
Copy link
Collaborator

ambv commented Mar 3, 2020

A full example of current behavior which we'd like to standardize:

To be clear, the example above does not demonstrate the behavior we'd like, only lists all cases that I find we need to take into account.

@ambv ambv changed the title Proposal: only enforce blank line separation between functions and classes at the same level of nesting Only enforce blank line separation between functions and classes at the same level of nesting Mar 3, 2020
@bhajneet
Copy link

I've done a cursory reading of this issue and related issues. I'm asking because this has been in discussion for almost 4 years:

How does a decision like this get made? Are there any actionable items that are required by the team? Will there be a consensus meeting? Etc.

Looking forward to Black having an opinion on this! (I also don't care which opinion, as long as there is a clear and deterministic one). 👍 Black is an awesome program, keep up the great work!

@felix-hilden
Copy link
Collaborator

Thanks! I think this issue still needs some discussion, but there's been related work on blank lines recently in #3035 and #2996.

Personally, I'd like to see them removed. And wrt. ambv's example (updated link that works), directly nested functions and methods have the newlines already removed. To me, the only thing I'd change (in addition to #3035 removing leading space in the def -> space -> some code -> space -> def case) would be the if cases. But, ambv might have a point with this being way better for short definitions.

@e-gebes
Copy link

e-gebes commented Jan 6, 2024

I think the original request does make a lot of sense to think about empty lines mainly as a means of separation inside one level of indentation. This is clear, unambiguous, and easy to understand.

Sometimes it makes sense to use empty lines regardless of the indentation, as outlined e.g. in #902 (comment)

Size of function/code

I'm not sure whether the size of an inner function definition should play a role. Long definitions hinder readability in general - regardless whether there are inner function definitions - and can be dealt with refactoring and extracting more smaller functions.

A bad example of "long code" but without inner function definitions:

class A:
    def method1(self):
        if abc:
            while True:
                if not dfe:
                    try:
                        if ghi is None:
                            for jkl in mno:
                                if pqr > 0:
                                    break
                    except:
                        continue
                x = 3
            y = 4
        z = 5 + self.method2(y)
        print("z calculated")

Such code is quite hard to grasp immediately, and I want to suggest that it does not have to be Black's goal to make such code better readable, but that the readability of edge cases and bad code is better left to the developer.

There is one thing the developer can improve easily, namely adding an empty line before the z = ... assignment. The whole long-indented if ... chain thing is then still unreadable, but at least the last two statements z = ... and the print(..) stand apart of the and seem to be a little bit better readable as above ("ok I don't see what this unreadable block is, but after it there are these two statements").

Similarly, Black can't do something about such structures that are even long and exceed screen pages, leaving indentation levels unclear at first grasp. Solution through refactoring, not auto-formatting.

A short example

# with and without empty line after the 'if' block:

def function():
    if abcd > 0:
        efgh = 1
    something = 2


def function():
    if abcd > 0:
        efgh = 1

    something = 2

Personally, I prefer the empty line version, and I mostly use that. It's opinionated, and situation-dependent, and Black leaves both versions as-is, and Black should keep doing so.
Let's think of the empty line as a separator between the if block and the something assignment, which are both on the same level of indentation. Makes sense (to me).

Why are function definitions special?

One might find it elegant to leave the separation with empty lines also in the case of (inner) function definitions to the developer, because after all, def statements and blocks are just statements and blocks like other statement and blocks.

Though, there are widely used guidelines like PEP 8 to separate/surround class and def statements on the global level by two empty lines. And: Method definitions are usually separated by one empty line. This exceptional, "mandatory" use of empty lines is historically grown, practically globally consistent and accepted, and makes sense given that most of these definitions in the real world are longer than just one- or two-liners.

So, it makes sense that Black enforces this, that's pragmatic and agreeable.

The exceptional treatment of class and def statements (over other statements/expressions) when it comes to adding empty lines makes full sense when taking only the vertical spacing into account - adding context about horizontal alignment (indentation) is not needed for the exceptional treatment to make sense.

I want to argue that in case of (horizontal) alignment differences, I don't see a clear or consistent approach, rule set, or community-wide usage that can be followed here. It's opinionated and controversial. Empty lines at the beginning of the block can now be added optionally, see #4043. I don't think they should be enforced (for def or class statements).

As well, I don't see good rationale for enforcing an empty line at the end of an indentation block only for class and def statements. I think Black should treat these cases the same way:

def function():
    try:
        if something:
            do()
    finally:
        pass


def function():
    try:
        def inner():
            do()
    finally:
        pass

@hauntsaninja
Copy link
Collaborator

hauntsaninja commented Jan 6, 2024

A lot of behaviour in OP's example no longer repros, so I think it's time to close this issue.

If someone here wants changes, I'd recommend opening a new issue with a summary of what behaviour changes from Black preview style on main in 2024 are desired.

If you do so, some requests:

  • Please link to this issue
  • Consider reading ambv's first two comments on this issue and the issue Newlines after code block open, once and for all? #4043 first
  • Just be aware it'll likely be an uphill battle (an invitation to open an issue is not a promise it will be accepted)
  • Arguments I find most persuasive are links to real world code where people feel Black's style makes things less readable

e-gebes, thanks for the writeup and concrete suggestion. I don't agree that those cases should be treated the same way. As ambv mentions, with def and class there's a different scope involved and Black's current style intentionally accentuates this.

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. T: style What do we want Blackened code to look like?
Projects
None yet
Development

No branches or pull requests

7 participants