What's up with the extra newline after return/yield/etc?

[njsmith]

It seems very idiosyncratic – are there any existing style guides that do this? I think of vertical whitespace as a separator, not a way of emphasizing the previous line. Why do you put the emphasis line after the control statement, instead of before it? Why do these statements in particular need extra emphasis? If they do, then wouldn't it be better to configure your syntax highlighter to wrap them in <b><blink>...</blink></b> or something? Should we ask Guido to rename the tokens to RETURN and YIELD to make sure no-one misses them? (Presumably during the transition period we'd have to do from __future__ import SHOUTY_TOKENS, but black could insert that for us.)

Of course all style guidelines are idiosyncratic and part of the reward for spending lots of time working on a tool like black is to make sure that it's your idiosyncratic preferences that everyone ends up using, so feel free to ignore this. But I figured I'd at least open an issue so people can click the little emoji buttons before it gets closed as WONTFIX.

[ambv]

If return is your last statement in a function, it already receives an extra empty line or two because of function separation.

If you escape early from a function or loop, this is easier to miss. This extra newline helps the reader to consider the change in flow. You're right that you can think of it as a separator from what comes after. After all, you shouldn't just keep reading as if it was still the same block.

Do those extra lines prevent you from doing something you could do otherwise?

[njsmith]

Do those extra lines prevent you from doing something you could do otherwise?

Nope, it's just a style preference, so de gustibus non disputandum and all that. (No idea if I spelled that right.) But looking at the black diff on trio right now it's responsible for a huge proportion of the changes, and imo it makes things slightly worse in basically every case. I already "use blank lines in functions, sparingly, to indicate logical sections", as pep 8 says, and it's great that black now lets me do that (#19), but it's not so great that it also introduces a bunch of extra lines at places that aren't logical section breaks. I think we agree that it's really valuable to be able to take in the high level structure of a function at a glance, and that black lines are a powerful tool for expressing that structure; my objection is that black's super-simpler heuristic for "structure" ends up creating a bunch of spurious signals that make it harder for my eyes to see the real structure.

I'm on my phone now, but I'll try to find some more concrete examples later.

[njsmith]

Okay, here's the promised examples.

There are a bunch of cases where the function is completely non-confusing and the extra line is pure clutter. For example, in both of these cases, you're seeing the whole function:

     async def wait_socket_readable(sock):
         if type(sock) != _stdlib_socket.socket:
             raise TypeError("need a socket")
+
         await wait_readable(sock)

    def capture(sync_fn, *args):
         try:
             return Value(sync_fn(*args))
+
         except BaseException as exc:
             return Error(exc)

In other cases, we have some genuinely complicated functions, like trio.run. The whole function body is too long to paste here, but you can see it here. If you follow that link, you'll see it already uses blank lines to divide the body into 4 logical sections: (1) pytest's magic __tracebackhide__ declaration, (2) error checking, (3) interpreting arguments and setting up global state, (4) a chunk of complicated error handling logic with nested try and with blocks wrapped around the actual work (and then the work itself is factored out into a separate function). And in that last part, black inserts a number of newlines:

    try:
        with ki_manager(
            runner.deliver_ki, restrict_keyboard_interrupt_to_checkpoints
        ):
            try:
                with closing(runner):
                    # The main reason this is split off into its own function
                    # is just to get rid of this extra indentation.
                    result = run_impl(runner, async_fn, args)
            except TrioInternalError:
                raise
+
            except BaseException as exc:
                raise TrioInternalError(
                    "internal error in trio - please file a bug!"
                ) from exc
+
            finally:
                GLOBAL_RUN_CONTEXT.__dict__.clear()
            return result.unwrap()
+
    finally:

This really doesn't make sense to me to read, like -- how can you have a "logical section" break in between two except clauses? Or between the contents of a try and the contents of its finally? For the existing logical sections I could easily describe their high-level purpose, but I can't do that for these at all.

Anyway, I know this is hair-splitting, hence the silly first message (I hope it came across as silly rather than whiney!). But it's genuinely unclear to me when this behavior would ever improve code – maybe you're used to working on codebases with over-complicated control flow that should be factored into multiple functions? 😜 If black didn't respect manual newlines, then it would at least feel coherent: it's taking full control over the vertical layout, and making the best guesses it can. But after e911c79 it feels inconsistent that black is sometimes trying to control section breaks inside functions, and sometimes letting the user control it.

[ambv]

how can you have a "logical section" break in between two except clauses?

Looking at the standard library, less than 1/3 of except: clauses have raise or return within. So in a majority of cases we don't break the flow when handling exceptions. An additional empty line showing that within a given block you do exit early helps differentiate between the two cases.

You asked why I put the empty lines after control flow statements. I do this because they break flow, so it seems natural to me they should also break visual flow of a function or loop. Why do I find it helpful? In code review I did over the last decade, I find that in changes that people make to code they didn't write (or don't remember), they surprisingly often make wrong assumptions about control flow. Or if they don't, the reviewers do. Combine this with insufficient testing and you get bad changes landing in the central repository.

And I don't think you actually disagree with this. Where I see the core of our difference in opinion is that you believe in forming strict "blocks" of code using empty lines which disallows any other vertical whitespace. Funnily enough, I personally have an even stricter opinion on this: "sections" within functions are a smell. I said this much in the comment you referred to. In it, I also mentioned that if you really need sections, placing standalone comments that describe what those sections represent is the way to go. In this situation, it's the comments that form the "block", not the empty line alone.

I agree with you that this was a message better sent when Black used to totally remove vertical whitespace from function bodies. But as expressed by others #19 (and quite a few conversations I had elsewhere), this is unpopular in the community and would seriously hurt the adoption of Black. It's not a hill I'm willing to die on.

On the other hand, I didn't get anybody else to object against making control flow statements prominent. I will need to gather more feedback. So far I'm pretty strongly in the "it's worth it" camp.

---

PS. If you want to see a pathological example of this style, look no further than in Black's own whitespace function. This is the part of the codebase I'm least proud of and will definitely refactor once the bigger rocks are dealt with. I actually like that the style highlights how ugly this function is. After all, it has a ridiculous cyclomatic complexity of over 70. Linus Torvalds would say it's in poor taste and I don't disagree.

[roganov]

I truly believe that the only sane way for black is to do nothing more than collapsing 2+ blank lines into one. I agree that blank lines are a smell, but I don't understand why blank lines after control statements are better. It isn't consistent IMO.

Meanwhile my nice little methods are getting broken up visually for no good reason:

    def _convert_product_id(self, row: RowProxy) -> int:
        try:
            return int(row["CODE"])

        except (TypeError, ValueError):
            raise IncompleteDataError(f"invalid product_id: {row['CODE']}")

[njsmith]

I figured out a shorter way to express my position here :-). It is:

Blank lines inside a function are effectively a kind of extra-terse comment. Comments are really tricky for a code formatter to handle, but they're crucial, because they're a way for developers to express intent and increase readability. Black shouldn't be in the business of adding or removing comments.

[ambv]

You win, @njsmith. Thanks for taking the time to clearly express your opinion.

FWIW, I still recommend making flow control statements prominent but I decided it's not worth alienating Black users over it.

[njsmith]

Oh, excellent! I'll have to try black again :-). And thanks for being open to considering these things!