-
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
Allow a single newline after opening blocks, possibly via an option setting #3551
Comments
The change is in fact in the changelog (the line is "Remove trailing newlines after code block open (#3035) (22.6.0)"). As you can see, the change has been in the preview style for six months, and I called it out as potentially controversial in #3407; it was also listed in https://ichard26.github.io/blog/2022/12/black-23.1a1/#remove-blank-lines-after-code-block-open. In all that time we didn't get complaints about this change. So I'm not very sympathetic to claims that this wasn't properly communicated. |
Sorry that I missed that. I only checked the first version 23 release notes, where we noticed it.
Apologies for that. We weren't aware of the preview style mode, and were caught by the change by surprise now. |
@bluenote10 I also upgraded to the newest version of black and saw 98% of my files change. @JelleZijlstra It's not always easy to keep up with changes that third-party libraries are making. I'm not saying to revert the change or anything but I think a single newline after opening blocks should be a TOML configurable option, especially since that's the way black has always done it up until now |
Hmm… I just added a comment #902 (comment) although that might be better suited here? |
My 5 cent: searxng/searxng#2159 (comment) |
@jenstroeger Yeah your comment is spot on and is the main issue my project is facing with the new formatting, I want new lines to stay above my comments. They are there for a reason |
@JordanZimmitti agree. I’m actually considering rolling back to the previous Black version because this collapsing of a single empty line to nothing makes code an eyesore to read. As much as I enjoy deploying Black, this change is too disagreeable for me and the arguments for it weak. @JelleZijlstra please consider this comment and ensuing conversation. |
@jenstroeger Yeah I did roll back, didn't want my newest commit to have 99% of the files changed |
Hello, |
@JelleZijlstra would you consider a PR for this? I don't think pinning black is an option - not for libraries that mostly don't pin dependencies (except a lower bound), and not in the long run anyway. |
You absolutely should pin Black (and other code analysis tools), or you'll eventually get inconsistent formatting. We could consider reverting this change, but only in 2024 for the stable style, and I'm hesitant to change back and forth since we already made this change in the stable style. |
Pin for the next 100 years ? :-) Hm, we upgrade with every version, we wan't to get in use of new features of code analysis .. and we fix the formatting issues reported in the project. I assume this is what most do ..
"Allow a single newline after opening blocks" .. is optional and by this it is downward compatible I think. |
I opened #3686 and am happy to get feedback. |
I'd be open to relaxing the check as follows:
This would go into the preview style now and in the stable style for 2024. |
I suppose this won't solve the problem why users have posted here in the thread / at least in my repositories I will have to touch thousands of lines of code to follow these strict rules (while the benefit of this formatting rule is questionable). |
@JelleZijlstra did you mistype that? I thought blank line after docstring was optional both on 23.3 and 23.3 --preview |
Oh yeah, I think it's only immediately after a block open, sorry for that. Will edit the above. |
I admit that I did not check the preview style but tried to be a good citizen and opened by opening #3686. I had been hopeful there for a moment... I do appreciate that you consider relaxing the conditions but fail to see why a newline is less relevant before code than a comment/ another compound statement. |
I wholeheartedly agree with the comments requesting reverting/relaxing #3035 and wonder how it was ever merged with such a thin rationale. According to the OP of #902, a leading empty line to separate an indented block "makes no sense". Well, if you accept this, then neither does a trailing empty line, yet those were not touched by the PR. The rule I follow goes like this: if a block contains empty lines, then also leave an empty line between the block's opening statement and its body. Can't use Black if it doesn't allow this. |
I agree with you. There is a long list of arguments against the discussed change but obviously reverting this is not a topic. Sounds like 'keeping a decision is more important than having made a better decision'. I had the hope that this will be reconsidered after a few months... did not happen until today. :( |
@LutzLuebbert, @bintoro or anyone else, instead of a lament, it might be more productive to open a PR implementing the behaviour Jelle said Black would accept in #3551 (comment) |
@hauntsaninja if I understand that statement from Jelle correctly it does not cover all the cases that are giving me a headache in my code (as it was also stated in #3551 (comment)). Starting to implement something that does not cover my case does not seem to be very productive as I have to dig my way through all the current code first while a revert of the discussed change gives the preferred result - but that is not part of the discussion / an acceptable path. And I really cannot see any benefits in adding more rules for something that was easy before. Having this said, there might be a lack of understanding on my part. Edit: Just saw this #3551 (comment) while scrolling up again. If Jelle can consider reverting as stated in the linked comment, I am confused that this (simple) option has been dropped from the discussion afterwards and we are talking about implementing new rules instead. I can imagine that no one here will ask for a revert of the revert again (next year). ;) |
Just to clarify: As far as I can see reverting this rule should not lead to any re-formatting for people who did switch to the current style (i.e. reformatted the code beginning of the year), because reverting the rule simply yields a super-set of allowed code, right? I.e., even if we revert people should not observe any kind of flip-flopping when re-applying black, it's just that they would now be allowed to use newlines in these places again if they want to. @JelleZijlstra Given that it would not break anything, would you be fine with a full revert? Otherwise, do you have some motivation why relaxing the rules only in certain cases is preferred? I don't really see a benefit to limit it to certain cases -- both from a readability and a implementation complexity perspective. The main argument brought forward here of "surrounding a block of statements symmetrically by 1-line of whitespace" still applies to other syntactical cases as well. For instance, why should a block of plain function call statements not be allowed to be surrounded by a symmetric 1-line margin, whereas an |
I just went over the original issue #902 and related discussion again. I still think we should remove leading newlines in some cases. As the original issue pointed out, we used to remove leading newlines in class blocks, but not in functions. That's inconsistent. I think it's good that we now turn def f():
return 1 Into: def f():
return 1 So I'd like to continue doing that. In my previous message, I outlined a rule that could prevent us from removing newlines in most of the cases that previous commenters complained about. Apparently that's not enough for some of you, but I haven't seen any further examples of code where removing newlines is ugly. As @bluenote10 said, it's true that technically turning off this rule wouldn't violate our stability policy, as already-formatted code wouldn't change. However, I put that escape hatch into the stability policy to allow us to fix unambiguous bugs (like cases where we deleted comments), and this isn't one of those. So if we make a change here, I think it can wait until 2024 (which in any case is only a few months away now). |
@JelleZijlstra thank you for re-reading the thread! While I agree with your example above, here is a related discussion with examples where it makes sense to keep these blank lines: if they’re followed by a comment. |
That's what I wrote in #3551 (comment), no? |
@JelleZijlstra you did. When you mentioned “I haven't seen any further examples of code where removing newlines is ugly.” I wanted to make sure we’re on the same page. |
Thanks for addressing this issue! However, after experimenting with #3967 for a while in multiple codebases I don't think it really resolves the actual problem. I'm still observing hundreds of changes that result in clearly less readable code. The vast majority comes down to the issue of "swallowing the return type" as demonstrated by @vonHartz in this comment: #902 (comment)
Incidentally, in my empirical analysis in multiple codebases this benefit had very few hits, whereas the detrimental effects were numerous. Single empty lines are a powerful tool that can be used to improve the readability or to harm it. Black allows them in many context anyway, and to some degree we have to trust the developers to not abuse the "tool". From empirical experience I have the impression that humans have a pretty good sense of making use of vertical margins/spacing in a surprisingly similar fashion, i.e., it is rare to come across code that uses single empty lines in a completely counter intuitive way like interleaving every line with a newline or so. I understand that black is all about removing freedom where it doesn't matter to avoid unnecessary discussions about equally "good" variations. But in this case the removed freedom does cause harm and still does after #3967: I do have trouble seeing my return values clearly, and I'm overlooking variable definitions that get shadowed by verbose functions signatures... |
Could you give concrete examples of code samples that look worse with the newlines removed? |
Here are the examples I gave in #902 . They matter for complex functions, especially with type hints. As @bluenote10 pointed out in the other issue, the block structure is much easier to infer with a newline. With it, it is way clearer where the signature ends and where the body begins.
And it gets worse for longer signatures.
|
While I appreciate your work, I'm afraid I have to agree with @vonHartz (#3551 (comment)) / IMO #3967 doesn't solve what the opener @bluenote10 in the initial comment asked for. The suggestion to pin the black version makes no sense in my opinion (#3551 (comment)) .. I really liked using "black", but unfortunately I can no longer use it in my projects without causing conflicts in all forks and branches .. cherry picking, merging, the diffs .. everything becomes more complex without any real benefit in return. I'm sorry if I'm putting this in a somewhat casual way, but I think, not to allow empty lines is an academic decision, that don't meet the needs of practice. |
The stubborness around this issue is comical. Why is the onus on the opposition to argue for rolling back #3035, when there was no compelling rationale for the PR to begin with, particularly given its impact? To me it seemed like the new rule was cooked up on a whim, without much thought given to whether it was even needed or the repercussions it would have. The opening comments of #902 and #3035 only provided a couple of toy examples that were then used to justify a sweeping change. Black is fine with splitting a function into logical blocks by inserting empty lines. It defies logic that similar spacing above the first block should be disallowed. That would be analogous to disallowing spacing between a heading and the subsequent paragraph but not between adjacent paragraphs. If authors can have no say as to what their code looks like, I would actually be in favor of doing the opposite of #3035 and mandating an empty line at the start of an indented block that itself contains empty lines. Would that be acceptable? If yes, I'll volunteer to write a PR. |
Blank lines are our paragraph breaks, we can't really identify paragraphs without them. How much space there should be between headings and first paragraphs and then between subsequent paragraphs are very definitely things a typesetting guide would have an opinion about. If I dig through the ticket history I wonder if I will find a conversation like this for every place where black makes an opinionated choice, I feel for the maintainers having to be in dozens of these threads. |
Absolutely. And specifying zero spacing after a heading would be a highly unconventional opinion if we talk about prose. Regarding code, I understand people have varying tastes on the matter. I wonder if there's a philosophy driving Black's development or if it's just a matter of who happens to submit a PR first to implement their own tastes. The rationales given in #902 and #3035 were that empty lines "make no sense" and "are redundant", respectively. Really? I'm fine with opinionated, but when the opinions start getting poorly justified, unnecessary, and downright harmful, I'm out. |
@JelleZijlstra It is a bit tricky, because it is mostly closed source, but a random example from a leisure project:
vs
The spacing between the
I'm pretty sure that one could conduct an experiment to actually quantify that it does takes a fraction of a second more time to mentally parse these two things without the space, due to very basic principles of how the human perception works.
Indeed, and note that it is a fundamental principle of basically any traditional typesetting guide to recommend spacing between a heading and the first paragraph. How often do you come across a text where the first paragraph is glued to the headline?
That is certainly true, and I feel bad for having this discussion. For what it's worth: I'm a black user for many years, and I think the maintainers have done an awesome job in coming up with great conventions so far. This is the first time I've encountered a decision that I personally deem detrimental. |
@JelleZijlstra wrote:
Wow, I hadn't even realized this. In Black v22, the following happens: Input:
Output:
Indeed, this is inconsistent. In fact, it's inconsistent even without bringing the treatment of function bodies and other indented blocks into it. I mean, why is the leading newline removed before I would argue that it would have been preferable to address this simply by allowing the leading newline in class |
I feel the empty lines give the code a bit of room to breathe. Two examples I find especially sore on the eyes.
I also want to echo @bluenote10 sentiment - I could not have said it better... |
Jelle recently asked me what my opinion on this was. I thought about it a little and have come to a tentative conclusion that we should walk back more than what we did in #3967 / #3551 (comment) Given that discussion is a little scattered, I opened #4043 with some summary and thoughts. Hopefully this isn't an annoying thing to have done. |
Is your feature request related to a problem? Please describe.
Previously black allowed to have a newline after a code opening black. This can indeed improve readability.
Compare for instance this
with
There are two reasons, why a single newline at the beginning of a block can improve the readability:
some_long_function_and_condition(a, b, c) == expected
feels overshadowed by thedef some_long_function_name(with_many: List[int], params: Optional[T]):
From a practical perspective: Going from black version 22 to 23 in our code base is causing a massive diff! The vast majority of the diff (perhaps ~95%) is coming from this very change, which isn't even listed in the release notes of version 23. Basically all the changes would make the code more ugly due to either of the two points above.
Describe the solution you'd like
From a design perspective, #3035 was probably a mistake. Since it is probably too late to revert that change, I'd hope that this is one of the few cases where we can introduce an option that allows the old behavior. Something like
allow_newline_at_beginning_of_blocks
.Describe alternatives you've considered
Perhaps even reverting #3035 ?
Additional context
Anything else needed?
The text was updated successfully, but these errors were encountered: