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 violates pep8 recommendation with long argument list #1178

Closed
stefanoborini opened this issue Nov 27, 2019 · 54 comments
Closed

Black violates pep8 recommendation with long argument list #1178

stefanoborini opened this issue Nov 27, 2019 · 54 comments
Labels
T: style What do we want Blackened code to look like?

Comments

@stefanoborini
Copy link

Currently, black reformats long routine names as follow

# in:

def very_important_function(template: str, *variables, file: os.PathLike, engine: str, header: bool = True, debug: bool = False):
    """Applies `variables` to the `template` and writes to `file`."""
    with open(file, 'w') as f:
        ...

# out:

def very_important_function(
    template: str,
    *variables,
    file: os.PathLike,
    engine: str,
    header: bool = True,
    debug: bool = False,
):
    """Applies `variables` to the `template` and writes to `file`."""
    with open(file, "w") as f:

Desired style

The current style done by black violates pep8 recommendation

# Add 4 spaces (an extra level of indentation) to distinguish arguments from the rest.
def long_function_name(
        var_one, var_two, var_three,
        var_four):
    print(var_one)

The logic and motivation behind pep8 formatting, and against black, is that one wants to keep the indentation of the function definition continuation at a different level compared to the logic block. Otherwise, it's harder to differentiate what's one and what's the other, despite the indented frowny face.

In fact, flake8 reports a pep8 violation for a case such as this

    if path.suffix in (
        '.js', '.json'
        ):
        proto = Protocol.json

x.py:20:5: E125 continuation line with same indent as next logical line

Black current formatting would be equivalent to this

    if path.suffix in (
        '.js', '.json'
    ):
        proto = Protocol.json

Which we can probably all agree is a bad idea.

Additional context from python-ideas mailing list

@stefanoborini stefanoborini added the T: style What do we want Blackened code to look like? label Nov 27, 2019
@asottile
Copy link
Contributor

the bottom of the section you linked shows this:

or it may be lined up under the first character of the line that starts the multiline construct, as in:

my_list = [
    1, 2, 3,
    4, 5, 6,
]

result = some_function_that_takes_arguments(
    'a', 'b', 'c',
    'd', 'e', 'f',
)

which I believe is in line with what black is doing

@stefanoborini
Copy link
Author

@asottile that's for when you invoke a function, not for when you define it. When you invoke a function, the problem of visual collision between the indented arguments and the following blocks does not exist, because the code that follows is always at the same indentation level as the function call.

@asottile
Copy link
Contributor

it's not immediately following and the same indentation though, black always puts a paren on the next line so they aren't immediately the same (and uses identical style for calls and definitions)

@stefanoborini
Copy link
Author

@asottile It doesn't matter. Why one would have to rely on a parenthesis that is at indentation level 0 to distinguish the declaration from logic that are both at level 1?

@ovidiu-munteanu
Copy link

To be honest, I much prefer the black style. It makes it a lot easier to read the parameters in cases where you have lots of them. Even in your first example I find the black approach to be preferable. So I would suggest the contrary, i.e. that the black style is maintained as is even if it is in breach of PEP8.

@stefanoborini
Copy link
Author

@ovidiu-munteanu I have the completely opposite view. It makes it harder to read and discern what is parameters and what is code.

@mk-pmb
Copy link

mk-pmb commented Dec 10, 2019

The nice thing about strongly opinionated code formatters is, it's so easy to make your own. ;-)

@stefanoborini
Copy link
Author

@mk-pmb not the point. If black wants to be opinionated, at least it should comply with pep8.

@mk-pmb
Copy link

mk-pmb commented Dec 10, 2019

Meanwhile I found we carry an "autopep8" tag in the Github repo tags. Indeed, that's inconsistent then.

@stefanoborini
Copy link
Author

stefanoborini commented Dec 10, 2019

@mk-pmb

confirmed

➜  core git:(add-enums) ✗ cat x.py 
def very_important_function(template: str, *variables, file: os.PathLike, engine: str, header: bool = True, debug: bool = False):
    """Applies `variables` to the `template` and writes to `file`. """
    with open(file, 'w') as f:


➜  core git:(add-enums) ✗ autopep8 --aggressive x.py
def very_important_function(template: str, *variables, file: os.PathLike,
                            engine: str, header: bool = True, debug: bool = False):
    """Applies `variables` to the `template` and writes to `file`. """
    with open(file, 'w') as f:

➜  core git:(add-enums) ✗ autopep8 --aggressive --aggressive x.py        


def very_important_function(
        template: str,
        *variables,
        file: os.PathLike,
        engine: str,
        header: bool = True,
        debug: bool = False):
    """Applies `variables` to the `template` and writes to `file`. """
    with open(file, 'w') as f:

@mintchkin
Copy link

Meanwhile I found we carry an "autopep8" tag in the Github repo tags. Indeed, that's inconsistent then.

I don't think this is meant to indicate that the tools are compatible, just that they're related. There are also tags for yapf and gofmt which are definitely not compatible with black in any way.

@JelleZijlstra
Copy link
Collaborator

Black isn't going to change this style now because it would be too disruptive for our users.

@stefanoborini
Copy link
Author

stefanoborini commented Dec 13, 2019

@JelleZijlstra Are you serious??? Reopen the issue right now or I am forking. This is a bug. Period. It does not comply with pep8. It does not comply with autopep8. It must be fixed.

@stefanoborini
Copy link
Author

@JelleZijlstra

This is in your documentation

NOTE: This is a beta product
Black is already successfully used by many projects, small and big. It also sports a decent test suite. However, it is still very new. Things will probably be wonky for a while. This is made explicit by the "Beta" trove classifier, as well as by the "b" in the version number. What this means for you is that until the formatter becomes stable, you should expect some formatting to change in the future. That being said, no drastic stylistic changes are planned, mostly responses to bug reports.

This is a bug.

@mintchkin
Copy link

@JelleZijlstra It does seem like "stability" is a strange argument, could you elaborate on why this one is off the table? There have been some very backwards incompatible changes made quite recently, which I thought was the whole reason for black's beta status.

If the actual reason is that it's a purposeful violation of PEP 8 because the curators of black don't like how it looks, then I think that should be explicitly stated.

@stefanoborini just to clarify, the real issue here is that black doesn't add an extra indentation level to argument strings, not the fact that it puts the closing parens and colon on its own line, correct? I.e. this would be PEP 8-compliant formatting:

def some_function(
        argument_one, argument_two, argument_three
):
    pass

@stefanoborini
Copy link
Author

@mintchkin Affirmative. It's the lack of additional indentation the core of the issue.
The dedented parenthesis is pep8 compliant (see Indentation):

The closing brace/bracket/parenthesis on multiline constructs may either line up under the first non-whitespace character of the last line of list, as in:

my_list = [
    1, 2, 3,
    4, 5, 6,
    ]
result = some_function_that_takes_arguments(
    'a', 'b', 'c',
    'd', 'e', 'f',
    )
or it may be lined up under the first character of the line that starts the multiline construct, as in:

my_list = [
    1, 2, 3,
    4, 5, 6,
]
result = some_function_that_takes_arguments(
    'a', 'b', 'c',
    'd', 'e', 'f',
)

Although it's "odd" to me because I've rarely seen it used for functions, I agree with the general principle that keeping end commas for all parameters and the parenthesis on its own line the diff is small in case of an additional parameter. So well done there.

@zsol
Copy link
Collaborator

zsol commented Dec 15, 2019

@JelleZijlstra Are you serious??? Reopen the issue right now or I am forking

Dear @stefanoborini please tone it down. This forum is for civil discussions where the above style is not tolerated.

@stefanoborini
Copy link
Author

@zsol I consider it offensive to close a bug just because someone doesn't feel like to fix it.

@stefanoborini
Copy link
Author

@zsol Meanwhile if you could reopen it that would be grand.

@mk-pmb
Copy link

mk-pmb commented Dec 16, 2019

In case the disagreement stays, there's also a less cumbersome solution than forking the entire project: You could make an auto-patcher if there is none yet (a program that assists in installing black with selected addons and/or modifications), then provide a mod.

@stefanoborini
Copy link
Author

@mk-pmb it should not be a mod behavior. The right behavior should be the default.

@zsol
Copy link
Collaborator

zsol commented Dec 17, 2019

You might have noticed that closing brackets are always dedented and that a trailing comma is always added. Such formatting produces smaller diffs; when you add or remove an element, it's always just one line. Also, having the closing bracket dedented provides a clear delimiter between two distinct sections of the code that otherwise share the same indentation level (like the arguments list and the docstring in the example above).

This is our reasoning for choosing this style. We're not convinced it's a violation of PEP 8 - its point is to make sure there's visual separation of the function arguments from the body. Dedenting the closing bracket is enough visual separation in our opinion - it looks like your opinion is different. That's perfectly fine.

The backwards incompatible changes highlighted above are examples when people convinced the core maintainers of Black about a better formatting style, so that option is generally open. So far the arguments in this issue haven't convinced any of us; if there are more - different - arguments, let them come. If not, please let it go and let's move on with our lives.

@stefanoborini
Copy link
Author

stefanoborini commented Dec 17, 2019

@zsol

We're not convinced it's a violation of PEP 8

It's literally written in the PEP and autopep8 does not format it that way. How can you argue that?

Dedenting the closing bracket is enough visual separation in our opinion - it looks like your opinion is different. That's perfectly fine.

It's not only my opinion. It's in pep8. Literally, written in the pep8.

If not, please let it go and let's move on with our lives.

Again, I am sorry but this is unacceptable. Either this is fixed, or I am forking. Period.

@zsol
Copy link
Collaborator

zsol commented Dec 17, 2019

Either this is fixed, or I am forking. Period.

You're saying that like it's a threat. This is open source software, maintained on github. There's a fork button, it's easy. That's how OSS works. Feel free to fork Black and I honestly wish you the best of luck maintaining your fork.

@stefanoborini
Copy link
Author

@zsol I say that because I cannot believe that you would ignore an explicit statement from pep8, and I don't think that anybody wants two competing formatters that reformat the code one against each other. Because believe me, I won't let this go.

@JelleZijlstra
Copy link
Collaborator

The explicit statement from PEP 8 is arguing against a different formatting choice than the one Black makes. Perhaps we could propose a clarification to the text of PEP 8 to make it clear that Black's choice is another acceptable option.

autopep8 (which you cited a few times) is just another third-party tool, and it has no more claim to be authoritative than Black does. If Black formats things differently than autopep8, so be it; users can choose which tool fits their needs better.

Finally, as @zsol said you're totally free to fork Black. We've had people fork Black in the past to enforce single quotes, change the default line length, and use tabs instead of spaces. If you choose to add yours, good luck!

@stefanoborini
Copy link
Author

Perhaps we could propose a clarification to the text of PEP 8 to make it clear that Black's choice is another acceptable option.

It's not acceptable. The point of indenting an additional level is exactly because pep8 intention is to separate visually the arguments from the rest via indentation:

# Add 4 spaces (an extra level of indentation) to distinguish arguments from the rest.

and it explicitly uses a case similar to what black does as a No example

# Further indentation required as indentation is not distinguishable.
def long_function_name(
    var_one, var_two, var_three,
    var_four):
    print(var_one)

indentation is not distinguishable, and the re-entering parenthesis can be visually confused at a glance with an empty space, rather than the division between the two. This is made even worse when instead of an empty): you have a case like ) -> None: which makes the break between the two even more muddy and visually undefined.

@mk-pmb
Copy link

mk-pmb commented Dec 18, 2019

We've had people fork Black in the past to enforce single quotes,

Nice! Do you remember their project and could share a link? I skimmed #118 and #594 but couldn't easily find it.

@zsol (Edit: Sorry for mixing up your posts! I actually meant…)

@stefanoborini As I understand it, there have already been several single feature forks, so I think it would be a nice service to future opinion leaders if you could make a patcher so they can maintain just mods instead of having to maintain an entire fork. Would also be nice for users to be able to mix and match.

@piotr-dobrogost
Copy link

I think @stefanoborini is right as PEP-8 states the following:

When using a hanging indent the following should be considered; there should be no arguments on the first line and further indentation should be used to clearly distinguish itself as a continuation line.

So clearly, according to PEP-8, the only way to distinguish continuation lines is to use further indentation and not to mess with the placement of parenthesis.

Btw, this was raised in the past in issue #48.

@allan-simon
Copy link
Contributor

allan-simon commented Jan 2, 2020

I for one really hope black continues to indent the way it does , the ambiguity example of @stefanoborini is solved in black by the fact that the closing ): is on a separate line

@stefanoborini
Copy link
Author

@allan-simon I am sorry but you are wrong, and black is wrong. There's no space for discussion or ambiguity. pep8 states it clearly. pylint flags it. autopep8 reformats it in the correct way. black can kick and scream and its proponent can say that the frowny face is all they need but they are wrong. They are wrong visually, and they are wrong according to pep8. End of story.

@allan-simon
Copy link
Contributor

allan-simon commented Jan 2, 2020

@stefanoborini , I never stated I was not wrong with regard to pep8 which I could not care less ( as pep8 is a recommendation, not a RFC MUST or SHOULD), I care about the size of the diff produced by people in my team, so with this regard, black is playing a wonderful job, having the closing ): on a separate play for me the role of visual delimiter with the body, and help reduce diff-size when arguments are added. So the purpose of my comment was to express my support to black maintainers.

but "wrong visually" is subjective (as stated previously) , end of the story. So I don't think your comment is bringing anything to the discussion, you've already showed to everybody your holy book (which is a recommendation), do you plan on replying until everybody bows to the one true way of having a not visually wrong code?

Unless you plan on writing a PEP that makes PEP-8 mandatory so that you can open a PR in cpython to reject any "visually wrong" code, I think you just have to agree to disagree with us, the internet is big enough for different opinions.

if I was to troll I would say that PEP-8 says that further indentation should be used when it's not distinguishable with the line below, with the "):" being on its own line, I can argue that it becomes distinguishable.

especially if you look at the page on multi-line if , where PEP-8 states that they recommends no specific solution but only propose some possibility.

So with these in mind, having black that is consistent on all multi-line statement (be it if statement, function declaration, function call etc.) seems even more rational to me.

@allan-simon
Copy link
Contributor

@JelleZijlstra now that I've read more carefully PEP-8 I agree with you with ambiguity and I have created in that sense a issue on pep repo
python/peps#1267

as the PEP-8 gives as acceptable this part of code

    # Add a comment, which will provide some distinction in editors
    # supporting syntax highlighting.
    if (this_is_one_thing and
        that_is_another_thing):
        # Since both conditions are true, we can frobnicate.
        do_something()

so per this example

def very_important_function(
    template: str,
    *variables,
    file: os.PathLike,
    engine: str,
    header: bool = True,
    debug: bool = False,
):
    """Applies `variables` to the `template` and writes to `file`."""
    with open(file, "w") as f:

should also be valid because , ): aside, at the very least it has the very same kind of comment given in example by pep-8 itself. so While using 2 indentation level is one possible way of solving it, now I see nothing in pep-8 that explicitly forbid black indentation as once again black has never proposed to indent like this

def long_function_name(
    var_one, var_two, var_three,
    var_four):
    print(var_one)

(which is indeed stated as wrong per pep-8)

@piotr-dobrogost
Copy link

piotr-dobrogost commented Jan 3, 2020

@zsol

We're not convinced it's a violation of PEP 8

From comment by Guido van Rossum:

I would prefer PEP 8 to explicitly discourage putting ): on a line by itself, since I find it ugly, Black notwithstanding.

Also, from comment by Brett Cannon:

The trick with style guides is the potential list of unacceptable formats is nearly infinite while documenting what is acceptable is tractable. So assume if something is not listed as acceptable then it isn’t.

@piotr-dobrogost
Copy link

@allan-simon

): aside, at the very least it has the very same kind of comment given in example by pep-8 itself

That's unfortunate that the example given by OP has a comment. The fact that PEP-8 shows a comment as one of possible solutions is completely irrelevant to issue raised here.

@allan-simon
Copy link
Contributor

allan-simon commented Jan 4, 2020

and as said, pep-8 is meant to python official projects, so while I agree that if Guido does not like this style, there's not much we can do for it be accepted as part of pep-8, you also have to agree that pep-8 is not meant to be for every single project

Remember that PEP 8 is the style guide for Python itself, and while the community often times chooses to use it as their own style guide, it isn’t meant to cover all cases.

Having said that, I don’t really care what the style is for the Python standard library itself (which is what PEP 8 is actually about, regardless of the fact that tools seem to think that it’s the “one true style” for all Python code)

So the root of this argument is that Black produces code that isn’t always in compliance with PEP 8. I say this is fine – if you use Black you don’t need PEP 8 to tell you how to format your code. (PEP 8 is still useful for other things like naming things.)

so I think the idea then is "black violates pep-8 but that's not a problem" (and anyway it was already the case with line length)

@zsol
Copy link
Collaborator

zsol commented Jan 4, 2020

Thanks for taking the time to bring this issue to discuss.python.org, it definitely cleared things up. I think it's time to change our readme to not say the Black style is a strict subset of pep8 anymore.

@mk-pmb
Copy link

mk-pmb commented Jan 4, 2020

Let's even add a list of known differences to PEP-8.

@allan-simon
Copy link
Contributor

seems fair to me to warn people with the key difference

@stefanoborini
Copy link
Author

stefanoborini commented Jan 4, 2020

@stefanoborini , I never stated I was not wrong with regard to pep8 which I could not care less ( as pep8 is a recommendation, not a RFC MUST or SHOULD)

pep8 is a recommendation with a sensible reason behind it to enforce.

I care about the size of the diff produced by people in my team

This has nothing to do with the matter under discussion. The matter under discussion is the level of indentation of the arguments, which would have no effect on the size of the diff.

I say it again as the discussion made it clear it's not gone through. I am not discussing the frowny face on a separate, isolated line, I am discussing the missing level of indentation of the arguments (and of the frowny face as a consequence)

so with this regard, black is playing a wonderful job, having the closing ): on a separate play for me the role of visual delimiter with the body, and help reduce diff-size when arguments are added. So the purpose of my comment was to express my support to black maintainers.

Black maintainers can keep the closing frown on a separate line, as long as it's indented.
I was never against the frowny face on a separate line.

but "wrong visually" is subjective (as stated previously) , end of the story.

Indentation matters, otherwise we would all still be coding in Fortran 77.

So I don't think your comment is bringing anything to the discussion, you've already showed to everybody your holy book

The holy book is an official python recommendation and is enforced by all formatters except black.

@stefanoborini
Copy link
Author

stefanoborini commented Jan 4, 2020

@allan-simon

now that I've read more carefully PEP-8 I agree with you with ambiguity and I have created in that sense a issue on pep repo
python/peps#1267

as the PEP-8 gives as acceptable this part of code

    # Add a comment, which will provide some distinction in editors
    # supporting syntax highlighting.
    if (this_is_one_thing and
        that_is_another_thing):
        # Since both conditions are true, we can frobnicate.
        do_something()

This is part of a larger set of examples: quoting

When the conditional part of an if-statement is long enough to require 
that it be written across multiple lines, it's worth noting that the 
combination of a two character keyword (i.e. if), plus a single space, 
plus an opening parenthesis creates a natural 4-space indent for the 
subsequent lines of the multiline conditional. This can produce a visual 
conflict with the indented suite of code nested inside the if-statement, 
which would also naturally be indented to 4 spaces. This PEP takes 
no explicit position on how (or whether) to further visually distinguish 
such conditional lines from the nested suite inside the if-statement. 
Acceptable options in this situation include, but are not limited to:
# No extra indentation.
if (this_is_one_thing and
    that_is_another_thing):
    do_something()

# Add a comment, which will provide some distinction in editors
# supporting syntax highlighting.
if (this_is_one_thing and
    that_is_another_thing):
    # Since both conditions are true, we can frobnicate.
    do_something()

# Add some extra indentation on the conditional continuation line.
if (this_is_one_thing
        and that_is_another_thing):
    do_something()

The stated text says that it takes no explicit stance, but proposes options to mitigate the visual conflict. Pycharm will favor the third option and flag the missing indentation level if not added.

@acnebs
Copy link

acnebs commented Apr 15, 2020

@stefanoborini PEP-8 states that the reason this is a concern is because some differentiation needs to be made between the function definition/signature and the function body. In contrast, Guido says he doesn't like Black's way of doing it because he thinks it is ugly, not because it makes the two concerns hard to distinguish.

The objective standard (that the function signature and the function body need to be easily distinguished) is met by Black by having the "frowny face" on a separate line. I have never had an issue distinguishing between the function signature/body with Black's style (in fact, I think it makes it easier than the PEP-8 alternatives, and much much better than yapf's default), and the editors I have used have never had an issue with folding that function (one of your initial claims). As such, I think it would be constructive for the conversation if you stopped pointing at examples which don't have the closing paren on a separate line as examples that condemn Black. They are not the same as Black's style and therefore are not useful to show why Black's style is "bad".

The subjective opinion (that the "frowny face" style is ugly) is just that – subjective. I (and many others here apparently) think it is a much more attractive way to handle this issue.

When a style is objectively better, I generally think Black should adopt it. When the impetus is user preference, I think Black should stick with the style it has. And in this case, there is even an objective argument for why Black's choice is better than the PEP-8 promoted one (smaller diffs).

@stefanoborini
Copy link
Author

stefanoborini commented Apr 15, 2020

@acnebs At this point I start to believe that either people can't read or are misunderstanding my point on purpose just to stir up drama.

Copied from my above comment:

I say it again as the discussion made it clear it's not gone through. I am not discussing the frowny face on a separate, isolated line, I am discussing the missing level of indentation of the arguments (and of the frowny face as a consequence)

I say it again more explicitly just because apparently it does not get through:

I DON'T GIVE A DAMN ABOUT THE FROWNY FACE.

I WANT ONE INDENTATION MORE

and please don't consider me as an ass by doing so, at this point I have the right of being annoyed, since apparently you don't want to read my point.

@acnebs
Copy link

acnebs commented Apr 15, 2020

Well the issue there is that with the frowny face on a separate line, you don't need that extra level of indentation to achieve the "objective" goals, and I think it is sensible for Black to not create extra whitespace unless it is actually required.

The signature is already easily distinguished from the body without adding extra white space, so why do it?

@stefanoborini
Copy link
Author

stefanoborini commented Apr 15, 2020

@acnebs because:

  1. it breaks automatic folding that relies on indentation
  2. it puts the frowny face at level zero between two sets of entities that are at indentation one
  3. in python visual level is fundamental on distinguishing nesting and scope, and it generally goes always at higher level of indentations as you progress into the code. This is a case where you go to a higher level of nesting but you prepend that happening with a lower level of indentation
  4. it violates pep8, meaning that you can't use e.g. flake8 to assess the code.

Justification that black reduces the number of spaces is useless. Once the code has been formatted with an additional level of indentation, that change will not be shown anymore. The same argument could be given by not having indentation at all in the arguments, but we don't do that, right?

While I can agree on a minimal diff size for vertical spaces (assuming it really is a major problem, I don't think it is) at least we should not ruin visual cues.

@acnebs
Copy link

acnebs commented Apr 15, 2020

  1. Folding Python code that relies purely on indentation was fragile to begin with and would break on a lot of valid code (maybe not PEP8 compliant code, but such a feature shouldn't be relying on PEP8 compliance anyway) – this is not Black's fault.
  2. Yes, this is true. I don't see, however, why this is a concern. Unless your argument list is 50 args long (it shouldn't be), it is always easy to see when entities at level 1 are function arguments with Black's style. Likewise, you know that anything after a "):" is a function body, wherever that "):" appears. I have never seen anyone get confused and think that the args are actually part of the body with Black's style. In fact, I think the nice thing about black's style is that it provides consistency with other situations – in basically all other parts of Python code, it is convention that the closing paren/bracket/curly brace should be at the same level of indentation as the initial part of the expression, e.g. when creating a list, calling a function, etc.
  3. Agreed. That is why Black indents the arguments even though it doesn't actually need to. You could have no indentation for the args, which given the parens would be interpreted just fine by the python parser. Instead, Black chooses to indent the args once to make it clear that these are part of the function definition started above (one indentation level back). But not twice, which is gratuitous whitespace.
  4. flake8 (pylint?) has no problem with my Black-formatted code. Can you provide an example of Black-formatted code that flake8 is complaining about?

I'll add my own additional point in favor of Black's style:
5. "Own-line Frowny Face" is better for type-hinting, as it makes function return types cleaner, as your return type is on its own line (in the middle of the face). e.g.

def add(
    first_number: int,
    second_number: int,
) -> int:
    return first_number + second_number

To my eye, this is much cleaner than the PEP-8 recommendation on this subject (which was authored before Python type-hinting existed, and I don't think has been revisited since then):

def add(
        first_number: int,
        second_number: int) -> int:
    return first_number + second_number

@stefanoborini
Copy link
Author

This:

def add(
        first_number: int,
        second_number: int) -> int:
    return first_number + second_number

Should be (1)

def add(
        first_number: int,
        second_number: int
    ) -> int:
    return first_number + second_number

or (2):

def add(
        first_number: int,
        second_number: int
        ) -> int:
    return first_number + second_number

I prefer 1 tbh. Both are pep8 compliant.

flake8 (pylint?) has no problem with my Black-formatted code.

autopep8 reformats it, and pycharm complains about it in some cases.

@vdwees
Copy link

vdwees commented Apr 15, 2020

@stefanoborini WRT folding, I like that I can fold a function params and implementation independently with black formatted code. So I am happy with the behaviour of black (which is more efficient with its use of whitespace), but (1) is also ok.

@acnebs
Copy link

acnebs commented Apr 15, 2020

In all your previous examples I think you said that you wanted the formatting shown by my second example, no? Regardless, I think (1) is harder to read than Black's, and (2) is much uglier in my opinion (but easier to read than 1). In all three cases, I think it is clear where the args are and where the body is. And since Black has already chosen one of the three options, and they all achieve the same goal, existing behaviour should win.

Re: formatters. So autopep8 is breaking something? I'm confused. Why wouldn't different formatters with different styles reformat your code when you use them both together? Doesn't seem like this is Black's fault.

And if PyCharm's linter is flagging this as incorrect, seems like it should be fixed there, as while this Black style does not conform to PEP8 (it's not one of the suggested styles), it doesn't violate it either (it's not one of the "don't do this" styles). And in my opinion a linter should be concerned with valid code, not PEP8 code. Focusing too much on PEP8 is a good way to miss the forest for the trees and ignore smellier parts of your code that are nevertheless "PEP8 compliant".

@stefanoborini
Copy link
Author

guys, I don't care, do what you want.

@jarrellmark
Copy link

I would like to express my support for @stefanoborini and option number (1) which is pep8 compliant, has the frowny face on a separate line, and visually separates the arguments from the function body.

@tweakimp
Copy link

To me, it seems a bad idea to indent something additional times.
That is like having stairs that suddenly have a step twice the height and depth. You dont want to walk on stairs like this.

@acnebs
Copy link

acnebs commented Apr 15, 2020

@jarrellmark I disagree. Having adjacent lines with different concerns at the same level of indentation is more confusing than Black's approach. With Black, you know that if adjacent lines are at the same level of indentation, they are part of the same block/scope. Option (1) puts the the end of the signature and the body at the same indent level (confusing), and option (2) puts the end of the signature at the same indent as the args. I dislike this second option for two reasons. The first is that with type-hinting, I think it is better to have the function args and the return type be very easy to separate visually. The second is that I think the homogeneity with other constructs is a good thing. e.g. how we do this:

my_list = [
    1,
    2,
    3,
]

rather than this:

my_list = [
    1,
    2,
    3]

The visual consistency between the two similar concerns (how to style a lists of things bookended in some form of paren/brace) is nice to have.

@ambv
Copy link
Collaborator

ambv commented Apr 15, 2020

When creating Black, I was really careful not to blatantly break PEP 8 compliance. Of course, the default line length is different but the rationale is broadly explained, to reiterate: "90-ish" seems like a better default, for instance due to class bodies moving everything by one indent in most files.

The sadface argument folding is possibly the only actual innovation in Black. It is also broadly explained, to reiterate:

  • it's consistent with other bracket pairs, including function calls;
  • it minimizes diffs;
  • it gives the most space for arguments because every argument is treated the same and you don't have to use double indentation to differentiate between the function signature and the function body;
  • and it gives the most space for the return type annotation.

I find examples missing type annotations to miss the point of this style. With types every argument needs quite a bit of space and it's easier to visually parse them if they don't waste space by extra indentation or unrelated brackets on the same line. Most importantly, with the return type annotation the sadface stops looking so unnatural.

Real example from black.py:

def format_file_in_place(
    src: Path,
    fast: bool,
    mode: Mode,
    write_back: WriteBack = WriteBack.NO,
    lock: Any = None,  # multiprocessing.Manager().Lock() is some crazy proxy
) -> bool:
    ...

PEP 8

At the time when I created Black, and the folding behavior has been used since the first alpha, my understanding was that it does not violate PEP 8 but that particular detail was simply underspecified.

It turns out that @gvanrossum does not like how the sadface looks and would prefer this to be explicitly forbidden in PEP 8. I'm in no place to disagree with him and I don't wish to start a long discussion on this particular topic. Personally I found the sadface style to be pragmatic, especially in the face of type annotations, and composing well with the rest of what PEP 8 is after. However, that's just one guy's personal opinion. If PEP 8 indeed gets updated to explicitly disallow this form of folding, we will update Black's documentation to explicitly list this as a difference with PEP 8.

Closing discussion here

As the other team members and I found the discussion style here to be stressful and sometimes bordering on disrespectful, I'll be locking this particular topic, in this issue and others, if it reappears.

@psf psf locked as too heated and limited conversation to collaborators Apr 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T: style What do we want Blackened code to look like?
Projects
None yet
Development

No branches or pull requests