-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Comments
the bottom of the section you linked shows this:
which I believe is in line with what black is doing |
@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. |
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) |
@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? |
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. |
@ovidiu-munteanu I have the completely opposite view. It makes it harder to read and discern what is parameters and what is code. |
The nice thing about strongly opinionated code formatters is, it's so easy to make your own. ;-) |
@mk-pmb not the point. If black wants to be opinionated, at least it should comply with pep8. |
Meanwhile I found we carry an "autopep8" tag in the Github repo tags. Indeed, that's inconsistent then. |
confirmed
|
I don't think this is meant to indicate that the tools are compatible, just that they're related. There are also tags for |
Black isn't going to change this style now because it would be too disruptive for our users. |
@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. |
This is in your documentation NOTE: This is a beta product This is a bug. |
@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 |
@mintchkin Affirmative. It's the lack of additional indentation the core of the issue.
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. |
Dear @stefanoborini please tone it down. This forum is for civil discussions where the above style is not tolerated. |
@zsol I consider it offensive to close a bug just because someone doesn't feel like to fix it. |
@zsol Meanwhile if you could reopen it that would be grand. |
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. |
@mk-pmb it should not be a mod behavior. The right behavior should be the default. |
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. |
It's literally written in the PEP and autopep8 does not format it that way. How can you argue that?
It's not only my opinion. It's in pep8. Literally, written in the pep8.
Again, I am sorry but this is unacceptable. 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. |
@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. |
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.
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! |
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:
and it explicitly uses a case similar to what black does as a No example
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 |
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. |
I think @stefanoborini is right as PEP-8 states the following:
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. |
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 |
@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. |
@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 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. |
@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 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 ,
(which is indeed stated as wrong per pep-8) |
From comment by Guido van Rossum:
Also, from comment by Brett Cannon:
|
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. |
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
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) |
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. |
Let's even add a list of known differences to PEP-8. |
seems fair to me to warn people with the key difference |
pep8 is a recommendation with a sensible reason behind it to enforce.
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)
Black maintainers can keep the closing frown on a separate line, as long as it's indented.
Indentation matters, otherwise we would all still be coding in Fortran 77.
The holy book is an official python recommendation and is enforced by all formatters except black. |
This is part of a larger set of examples: quoting
# 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. |
@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). |
@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 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. |
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? |
@acnebs because:
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. |
I'll add my own additional point in favor of Black's style: 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 |
This:
Should be (1)
or (2):
I prefer 1 tbh. Both are pep8 compliant.
autopep8 reformats it, and pycharm complains about it in some cases. |
@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. |
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 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". |
guys, I don't care, do what you want. |
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. |
To me, it seems a bad idea to indent something additional times. |
@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. |
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:
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 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 8At 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 hereAs 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. |
Currently, black reformats long routine names as follow
Desired style
The current style done by black violates pep8 recommendation
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
Black current formatting would be equivalent to this
Which we can probably all agree is a bad idea.
Additional context from python-ideas mailing list
The text was updated successfully, but these errors were encountered: