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

One space between class definition and the first inner method #619

Closed
cmin764 opened this issue Nov 24, 2018 · 18 comments · Fixed by #4130
Closed

One space between class definition and the first inner method #619

cmin764 opened this issue Nov 24, 2018 · 18 comments · Fixed by #4130
Labels
F: empty lines Wasting vertical space efficiently. R: rejected This will not be worked on S: needs discussion Needs further hashing out before ready for implementation (on desirability, feasibility, etc.) T: bug Something isn't working

Comments

@cmin764
Copy link

cmin764 commented Nov 24, 2018

Can Black have a rule to not remove the whitespace at line 2 from here:

class C:
    
    def __init__(self):
        pass

so that it will keep the blank line between class definition and any first inner entity (as it does already with the docstrings and assignment attributes)?

It's extremely ugly to have that whitespace consistent in the entire codebase and just in this particular situation, where you don't have anything to add between class definition and the first inner method (which usually is the __init__ one), to end up with "glued" headers of class and method definition.

Now if I'm formatting this:

class C:
    
    def __init__(self):
        pass
    def func(self):
        pass

I get this:

class C:
    def __init__(self):
        pass

    def func(self):
        pass

So I expect the rule of one empty line before and after class methods to be consistent here. Everyone likes to have symmetry in their code.

@Delgan
Copy link

Delgan commented Nov 24, 2018

What a coincidence, I used black for the first time a few minutes ago and I had the same remark. I didn't bother opening an issue though because I thought it has been suggested before but refused for some reason?

PEP8 states that:

Method definitions inside a class are surrounded by a single blank line.

So I guess this applies for the first method too, including __init__().

@zsol zsol added T: bug Something isn't working F: empty lines Wasting vertical space efficiently. labels Feb 14, 2019
@pekkaklarck
Copy link

Just tested how black would format Robot Framework source code and this was the biggest problem. I found it important to surround methods with an empty line and think it should be done consistently. I understand that the motivation behind black is remove the need to discuss about different formatting alternatives, but in this particular I think black just behaves wrong. As already pointed above, PEP-8 explicitly states that

"Method definitions inside a class are surrounded by a single blank line."

In my opinion the PEP-8 compatible behavior should be the default. There could be a command line option to omit the empty line to avoid changes with existing blackened code bases. Having an option to preserve/add it would be the minimum.

@merwok
Copy link

merwok commented Dec 20, 2019

I think black’s behaviour make sense for classes that start with a docstring or comment. In this case, the empty line is wanted:

class C:
    """Abstraction for B to do spam."""

    def __init__(self):
        ...

@pekkaklarck
Copy link

pekkaklarck commented Dec 20, 2019

@merwok I totally agree there should be an empty line after the docstring. I think the docstring PEP also explicitly says so. This issue is about black removing the empty line if there's no docstring, even though that's against PEP-8. Without the docstring, black would format you example like this:

class C:
    def __init__(self):
        ...

@ambv
Copy link
Collaborator

ambv commented Mar 3, 2020

This is interesting. While your reading of PEP 8 is correct to the letter, I'm not quite sure if it was intended by the PEP 8 authors. Case in point: the PEP also says the following:

Surround top-level function and class definitions with two blank lines.

Following your logic, if a Python file starts with a function or a class definition, we would have to put two blank lines before it, no?

I'd like to understand your need for this blank line better. What purpose does it serve? There's clear delineation due to the increased indentation. In fact, I'm leaning towards implementing #450 so that we are consistent in that regard.

@merwok
Copy link

merwok commented Mar 3, 2020

if a Python file starts with a function or a class definition, we would have to put two blank lines before it

pep8 or flake8 used to require that and it was annoying!

@merwok
Copy link

merwok commented Mar 3, 2020

FWIW my take on the question here:

class CoolRegularClass:
    """Nice docstring.

    More info.
    Cool.
    """

    def __init__(self):
        # nice to have one blank after the docstring
        ...


class SomeInternalClass:
    def __init__(self):
        # no docstring, no needless blank line
        ...

In other words: the class statement is directly followed by the first interesting thing (docstring, attribute, method), then usual blank lines conventions applies.

@pekkaklarck
Copy link

Some reasons why I prefer an empty line before the first method in a class:

  • There are empty lines above other methods as well. I like consistency in this regard.
  • PEP-8 says so. There are likely lot of projects following that rule and Black forcing other style causes unnecessary code churn (or just prevents taking the tool into use).
  • I find it odd if docstrings or class attributes change this behaviour.
  • For my eye it just looks better.

I guess this is something where opinions differ and in my opinion stuff like this should be configurable. I know Black tries to be uncompromising, but there's already similar --skip-string-normalization option. It would be awesome if projects could have their own black.toml for options like this. Black could set the overall rules and let project decide on nuances like this. Black doing all formatting automatically would give you its normal benefits within a project. Two projects not having exactly same styles shouldn't be a big problem.

@gdub
Copy link

gdub commented Jun 30, 2021

Reasons for blank line, IMHO:

  • I interpret PEP 8's:

    Method definitions inside a class are surrounded by a single blank line.

    ...to include the first method.

  • Reduces diff size by one when adding/removing a docstring or class attribute(s), i.e. no blank-line insertion/removal churn.

  • Would make consistent with current behavior for leading class attributes or function calls. Currently, these are left as is:

    class LeadingAttributeAfterBlankLine:
    
        a = 1
    class LeadingFunctionCallAfterBlankLine:
    
        print("defining")

    ...but these classes with a leading method/class have the blank line removed:

    class LeadingMethodAfterBlankLine:
    
        def something(self):
            pass
    class InnerClassAfterBlankLine:
    
        class Inner:
            pass

    If going to remove blank line, then I'd probably prefer the opposite in these cases, i.e. blank line removed for leading class attributes and kept for method/class blocks.

Instead of forcing blank line or not, what if it were just left alone? I.e. allow zero or one blank line, as is currently done for the LeadingAttributeAfterBlankLine example.

@felix-hilden
Copy link
Collaborator

I'd prefer our current formatting: playground. Which is no empty line before functions, but one before other body statements. After discussing with others as well, we'll not be making any changes here.

@ichard26 ichard26 added R: rejected This will not be worked on S: needs discussion Needs further hashing out before ready for implementation (on desirability, feasibility, etc.) labels Jan 29, 2022
@ichard26
Copy link
Collaborator

@merwok
Copy link

merwok commented Jan 29, 2022

What is the private discord link for?

@felix-hilden
Copy link
Collaborator

It's a link to a public channel where we discuss development-related things.

@ichard26
Copy link
Collaborator

yeah my bad for not providing context, we are partnered with Python Discord (https://discord.gg/python) so we have our own #black-formatter text channel there for runtime communication.

@pekkaklarck
Copy link

It's unfortunate you don't want to make this configurable for projects like you've done at least with quotes. Being able to configure should there be empty lines or not, or at least having an option to just leave them as-is, would have been great. I like the idea of uniform formatting a lot, but I consider it a lot more important within a project than with all different Python projects. Having some things carved to stone is fine as well, but at least with behavior where PEP-8 is ambiguous configuration would make sense. Anyway, it's your project and your rules. For me this just means I won't be using Black.

@bintoro
Copy link

bintoro commented Nov 3, 2023

This unfortunate rule seems to be based on a flawed reading of PEP 8.

PEP 8 states:

Method definitions inside a class are surrounded by a single blank line.

To me, this directive seems pretty unambiguous, but the maintainers have interpreted it as:

Consecutive method definitions are separated by a single blank line.

This, of course, is not the same thing. It turns out that the directive actually used to say “separated”, but in 2015 it was specifically changed to “surrounded”. See https://hg.python.org/peps/rev/5784.

Thus, the current wording appears to be quite deliberate and, in all likelihood, is intended to apply to the first method definition as well. I mean, why wouldn’t it be?

[...] if a Python file starts with a function or a class definition, we would have to put two blank lines before it, no?

This argument is disingenuous. The purpose of vertical whitespace is to separate elements of code from one another. The rules governing it are not meant to apply at either end of a file.

I have read the prior discussion, and perhaps part of the problem is that the examples considered tend to be stubs and not representative of real code.

class DummyHelperClass:
    pass

Yes, I would agree that inserting a blank line there would be silly and ugly. But as soon as the class body grows to contain methods or other blocks that are themselves separated by blank lines, the leading blank line also becomes essential.

I hope that the above historical finding lends enough support to the literal interpretation of the PEP 8 directive in question that the decision to reject this issue can be reversed. Compliance with PEP 8 would seem to require that blank lines between adjoining class and def statements should not only be allowed, they should be mandated. If that is unpalatable, then authors should at the very least be given discretion to follow their chosen convention.

This whole thing popped up on my radar because the rule discussed here is now being used as an argument in favor of preserving PR #3035, which has proved a controversial and disruptive change.

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 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.
@e-gebes
Copy link

e-gebes commented Jan 6, 2024

This unfortunate rule seems to be based on a flawed reading of PEP 8.

PEP 8 states:

Method definitions inside a class are surrounded by a single blank line.

[...]

Thank you for the whole of the comment - it's well written, the historical finding is interesting, the interpretation of it and the argumentation built upon it is funny to some extent.

There are a lot of different code styles out there, many are inconsistent, some have written rules about them ("guidelines"), some of these are published to the general public, and possibly there are even some guidelines out there that are unambiguous.

What is Black? Published on its web page:

Black is a PEP 8 compliant opinionated formatter with its own style.

What is PEP 8? What is PEP 7? When have these two PEPs been introduced, to apply to which projects (answer: CPython itself), for which purpose?
How are they applied to "their" project? (I think this plays a role when interpreting and understanding single rules of PEP 8, though no one cares to look at the Python code of the CPython project how they eat their own dog food and take that into account when arguing about and interpreting excerpts of PEP 8.)
How is PEP 8 seen today by the community and understood what it "is"? (answer, over-exaggerating: strict standard for everything, basis for all other tools and guidelines)
How is PEP 7 understood by the community? (answer: practically no one knows about PEP 7. Why shouldn't play PEP 7 a bigger role in the whole C++ world, like PEP 8 does in the Python world?)

The point I am aiming to make here: Given the historical background of PEP 8, it might not be wise to take single phrases out of it and to try to interpret it to the letter. It might not be wise for Black to refer as first thing to it, because it is generally vague and generally misunderstood by large parts of the Python community - leading to a lot of irritating discussions about nothingness. Maybe Black should put the "PEP 8 compliant" label better into their FAQ, along with explanation that PEP 8 is vague, depends on interpretation, and, importantly, PEP 8 just cannot be interpreted to the letter, given the important - and often unnoticed - starting sections "Introduction" and "A Foolish Consistency is the Hobgoblin of Little Minds".

One just cannot be strict about PEP 8, there's the hobgoblin section!

Let's keep the section in mind whenever we read or write or think about PEP 8.

To me, this directive seems pretty unambiguous, but the maintainers have interpreted it as:

Consecutive method definitions are separated by a single blank line.

This, of course, is not the same thing. It turns out that the directive actually used to say “separated”, but in 2015 it was specifically changed to “surrounded”. See https://hg.python.org/peps/rev/5784.

Thus, the current wording appears to be quite deliberate and, in all likelihood, is intended to apply to the first method definition as well. I mean, why wouldn’t it be?

The message that came along with the linked change set is unfortunately just "Separate -> surround."
This doesn't give us insight in the motivation of the phrasing change. To interpret the phrase, it would be best to interview the people involved back then: the author of the phrasing change, the devs and reviewers who have tried to and were obliged to follow the (rephrased) PEP 8, and so on. And then there's always also the hobgoblin section...
We can of course make an interpretation on our own, like you did. If I would have to think of a way how it would be possible to interpret the change of the phrasing in a different way then you did, I can imagine this:

The "separate" phrasing, when read to the letter, could be imagined to apply only in cases where one method follows another method. But not in cases where there is a method, then one or more class attributes, and then another method definition. By using the wording "surrounding", when read to the letter, would then also have to be applied to method-attribute-method cases.

In the presence of the hobgoblin section, it does not matter much which of these interpretations, or both, or none of them are accurate.

[...] if a Python file starts with a function or a class definition, we would have to put two blank lines before it, no?

[...] The purpose of vertical whitespace is to separate elements of code from one another. The rules governing it are not meant to apply at either end of a file.

I think the argument wanted to provoke reflection about the interpret-PEP-8-to-the-letter approach. It resulted in applying opinionated common sense rather than literal interpretation. This is revealing, and gives a clear hint that this whole issue is just about a formatting opinion. There's nothing wrong with that, but let's rather be clear about it.

@bintoro
Copy link

bintoro commented Jan 6, 2024

Any hopefuls still following this issue will be delighted to learn that it has finally been fixed by PR #4130.

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. R: rejected This will not be worked on S: needs discussion Needs further hashing out before ready for implementation (on desirability, feasibility, etc.) T: bug Something isn't working
Projects
None yet