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

Newlines after code block open, once and for all? #4043

Closed
hauntsaninja opened this issue Nov 13, 2023 · 8 comments · Fixed by #4060 or #4130
Closed

Newlines after code block open, once and for all? #4043

hauntsaninja opened this issue Nov 13, 2023 · 8 comments · Fixed by #4060 or #4130

Comments

@hauntsaninja
Copy link
Collaborator

hauntsaninja commented Nov 13, 2023

Background:

(there's discussion elsewhere too, but I think these are the most important links)


tldr; the 2023 stable style strips newlines after code block opens. This has proven controversial (and was most of the change from 22 to 23 by volume). Recently, we walked this back a little bit in preview style, but we should try to get it right for 24.

The main argument against is it can hurt readability, see #902 (comment) . I'm quite sympathetic to this. For instance, functions with a complicated return type and first statement can get quite dense. Similarly, there are examples with comments in a new code block where things can get too dense.

An auxiliary argument against it is that stripping new lines between class and first method is in violation of a close reading and changelog analysis of PEP 8, see #619 (comment). I don't care much for PEP 8 absolutism (and certainly not at the expense of churning established behaviour), but I consider it a nice-to-have to not reformat away from it.

Recently in #3967, Henri contributed a nice change that walks this back in some cases, involving comments or compound statements. However, I'm not sure this is enough. Here are two examples from the first few pages of Black 22 -> 23 diff in my work codebase where I feel readability really suffers (scrolling through I see many others). Henri's change won't save us here. And fundamentally this code is quite simple, it just has slightly long type and variable names, i.e. it's not the kind of code where there should have been a comment or something.


Jelle nudged me to think about this, and I now feel we should just walk this change back entirely. We should allow users to have either 0 or 1 blank newlines. This is also simpler and more explainable than the partially walked back logic.

I am fine with continuing to strip blank newlines between a function declaration and its docstring (and preview does this for classes as well). We currently enforce one blank newline after class and module docstrings. We don't have an opinion on blank newlines after function docstrings (see #2370). I think this is probably fine as-is and not having an opinion would be consistent with the proposed no docstring case. (I'll grant that it feels easier to come up with a good opinion for the post docstring blank line in functions. At the very least, we certainly should not resolve that issue by always removing blank lines).

This will not create any churn going from 23 -> 24 (in fact, most of this change technically even meets our stability policy (although any change here should certainly only be made in preview style)).


As a meta thing, as a relatively new maintainer of Black, I'm still unsure of my decision making framework. I feel the original change had at most marginal benefit, especially given its churn (I've never browsed code and been like ugh one extra blank line, nor has any code reviewer or linter ever yelled at me for it. These are the things Black usually saves me from. All the motivating changes I found have multiple blank newlines or are toy code). On the other hand, it's very costly to revert changes in stable style. The only thing that makes it possible in this case is that both unreverted and reverted formatting will be valid and inconsistency in this specific case is not costly.

@mcnoat
Copy link

mcnoat commented Nov 13, 2023

I would support a blank line after the definition of functions without a docstring. I find the two examples provided convincing in that regard.


I am fine with continuing to strip blank newlines between a function declaration and its docstring

I think this behavior should be kept, too. As I've said in #2370 (comment), having a blank line after a docstring would be in violation with all of the major Python docstring conventions (PEP 257, numpydoc and Google). Pydocstyle files "No blank lines allowed after function docstring" under error code D202. D202 is included in all the docstring conventions mentioned above (source: http://www.pydocstyle.org/en/stable/error_codes.html#default-conventions).
My guess would be that it's not uncommon for projects to use both pydocstyle and black and in that case the two should ideally not be in conflict with each other.

@JelleZijlstra
Copy link
Collaborator

OK, let's walk back the change.

@mcnoat reading your comment, there might be confusion between blank lines before and after docstrings? Given this function:

def f():
    
    """
    doc
    """
    
    hi

Currently we remove the blank line before the docstring (and Shantanu says we should keep doing that; I agree), but not the one after the docstring. Possibly we should remove the one after the docstring, but that's a discussion for #2370.

@JelleZijlstra
Copy link
Collaborator

As a meta thing, as a relatively new maintainer of Black, I'm still unsure of my decision making framework.

I feel like I owe you a response to this, but I'm not sure I have a good one :) Some of the things I look at are:

  • How disruptive is this change? I generally want to make changes to the stable style only if they feel like clear improvements; when people upgrade Black and see the changes, they should feel it made things better.
  • Do I like the change? That's obviously subjective, but I feel part of the point of Black is that instead of the entire Python ecosystem arguing over style decisions, it's just us on this tracker who have to do that.
  • Does it feel like the community supports the change? For example, if there are lots of people who comment/like comments in favor of a change, that's a good signal.
  • Does the change feel consistent with Black's overall style? Here's a couple of principles that I feel we generally follow, though some may be controversial:
    • No backslashes for formatting, ever
    • Indentation should always be by a multiple of four
    • Formatting should generally not depend on previous formatting, though with obvious exceptions like the magic trailing comma
    • Diffs should be easy to read
    • Fewer lines is better than more

In this particular case, I think a few of us simply misjudged how controversial this change would be. Personally I liked most of the changes Black 22 -> 23 made to our work codebase; many looked like this:

 def _source_query_sql_fn(dt: str) -> str:
-
     sql = f"""
         SELECT DISTINCT

Where the extra newline just felt like a waste of space.

Hopefully to prevent a repeat of this experience, we can get more eyes on the current preview style (thanks for opening an issue for that!) before promoting it to stable.

@mcnoat
Copy link

mcnoat commented Nov 15, 2023

@mcnoat reading your comment, there might be confusion between blank lines before and after docstrings? Given this function:

You are absolutely correct, thanks for pointing it out. Since hauntsaninja left a comment on #2370, I presumed that this issue was about the same thing and - despite honest effort - did not read his comment or the original post here close enough.

@bintoro
Copy link

bintoro commented Nov 29, 2023

This is great news. Thanks guys.

One thing though: #4060 does not fix #619. That pre-#3035 behavior will now remain the odd man out.

@JelleZijlstra
Copy link
Collaborator

Yes, we still remove the first blank line here:

class Cls:

    def method(self):

        pass

That seems fine to me.

@bintoro
Copy link

bintoro commented Dec 10, 2023

Right, I had gotten the impression that addressing the def after class case was part of the plan, as the initial comment specifically referenced #619.

And from this comment I understood that the inconsistency bothered @JelleZijlstra as well.

Yes, we still remove the first blank line here:

class Cls:

    def method(self):

        pass

So we get:

class Cls:    
	def method(self):

		pass
  • From a typographic perspective this doesn’t look logical or visually satisfying.
  • This goes against PEP 8 as explained in #619.
  • Were the first inner statement a class variable instead of def, the empty line would not be removed.
  • Empty lines preceding subsequent def statements are not removed (in fact, they are enforced).

I’m struggling to understand the motivation for special-casing a class-adjacent def and treating it differently than everything else. This certainly feels like a bug even if it’s planned.

hauntsaninja added a commit to hauntsaninja/black that referenced this issue Dec 28, 2023
Fixes psf#4043, fixes psf#619

These include nested functions and methods.

I think the nested function case quite clearly improves readability. I
think the method case improves consistency, adherence to PEP 8 and
resolves a point of contention.
@hauntsaninja
Copy link
Collaborator Author

hauntsaninja commented Dec 28, 2023

Thanks @bintoro for following up! It was indeed my intention in this issue that we allow empty lines between class and method (although I personally don't care the most about this case). We also should allow new lines before a nested function (I find this case can clearly help readability). #4130 should fix both of these cases.

hauntsaninja added a commit to hauntsaninja/black that referenced this issue Dec 28, 2023
Fixes psf#4043, fixes psf#619

These include nested functions and methods.

I think the nested function case quite clearly improves readability. I
think the method case improves consistency, adherence to PEP 8 and
resolves a point of contention.
JelleZijlstra pushed a commit that referenced this issue Jan 1, 2024
Fixes #4043, fixes #619

These include nested functions and methods.

I think the nested function case quite clearly improves readability. I
think the method case improves consistency, adherence to PEP 8 and
resolves a point of contention.
hauntsaninja added a commit to hauntsaninja/black that referenced this issue Feb 18, 2024
Reflects status quo following psf#4043

Fixes psf#4238
hauntsaninja added a commit that referenced this issue Feb 26, 2024
Reflects status quo following #4043

Fixes #4238
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants