-
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
Long with statements are not broken into several lines #664
Comments
I suppose this is Black strongly dislikes backslashes. I'm not sure there's really a great formatting Black can pick here. |
I understand and agree with this. However, the situation with long Another example of problematic black formatting : with open('verylongfilenamethatcannotbesplit','r') as f, \
open('verylongfilenamethatcannotbesplit','r') as f, \
open('verylongfilenamethatcannotbesplit','r') as f, \
open('verylongfilenamethatcannotbesplit','r') as f, \
open('verylongfilenamethatcannotbesplit','r') as f, \
open('verylongfilenamethatcannotbesplit','r') as f, \
open('verylongfilenamethatcannotbesplit','r') as f:
print('Hello') becomes: with open(
"verylongfilenamethatcannotbesplit", "r"
) as f, open(
"verylongfilenamethatcannotbesplit", "r"
) as f, open(
"verylongfilenamethatcannotbesplit", "r"
) as f, open(
"verylongfilenamethatcannotbesplit", "r"
) as f, open(
"verylongfilenamethatcannotbesplit", "r"
) as f, open(
"verylongfilenamethatcannotbesplit", "r"
) as f, open(
"verylongfilenamethatcannotbesplit", "r"
) as f:
print("Hello") |
I for one have a deep hatred of backslashes and would really prefer black never use them. And since python's syntax here doesn't really allow for wrapping, i dont think black should do anything about it. I would argue repeated |
If developer A wants to format it a different way than developer B in order to prevent wrapping, that's fine. But it shouldn't affect the overall goal of a formatter –– doing the best with that it's given without affecting the AST. In essence, Black shouldn't just ignore and not make lengthy
|
+1 for backslashes in this context |
Another case to consider, it's fairly common to mock stuff like this: with mock.patch('mod1.mod2.qname', return_value=1), mock.patch('mod1.mod2.qname', side_effect=[1, 2, 3, 4, 5]):
assert [1, 2, 3, 4, 5] == my_mod.tested_func() Blackened code is slightly unusual (same as example with open above) and IMHO could be improved. with mock.patch("mod1.mod2.qname", return_value=1), mock.patch(
"mod1.mod2.qname", side_effect=[1, 2, 3, 4, 5]
):
assert [1, 2, 3, 4, 5] == my_mod.tested_func() |
Also +1 for backslashes. When there are a lot of chained context managers that have arguments, the black formatting really decreases readability. In the example from @rogalski it is very easy to overlook that there is more than one context manager used. |
Imho most of these would look nicer with contextlib.ExitStack |
This is the same issue as #557 ; see discussion there. |
Issue is indeed longstanding. #412 was already closed, which is not a good sign. It seems that we will have to wait for python to allow wrapping contents of Here is the corresponding python issue: https://bugs.python.org/issue12782 |
I changed my mind on this. Let's special-case Rationale: not only is it not possible to format this nicely in all versions of Python up to and including 3.8, it's also not possible to create a simple LL1 grammar rule to support parentheses for this use case. |
In the spirit of documentation-driven development, this is a proposed section of the docs explaining this cop-out: Backslashes and context managersBlack dislikes backslashes and removes them on every occasion. For We don't want formattings like: with make_context_manager1() as cm1, make_context_manager2() as cm2, make_context_manager3() as cm3, make_context_manager4() as cm4:
... # nothing to split on - line too long or worse yet: with make_context_manager(1) as cm1, make_context_manager(
2
) as cm2, make_context_manager(
3
) as cm3, make_context_manager(
4
) as cm4:
... # very questionable tokens to split on Ideally we'd like the last example to be formatted like this: with (
make_context_manager(1) as cm1,
make_context_manager(2) as cm2,
make_context_manager(3) as cm3,
make_context_manager(4) as cm4,
):
... # sadly not valid Python syntax For lack of a better option, Black will now format it like this: with \
make_context_manager(1) as cm1, \
make_context_manager(2) as cm2, \
make_context_manager(3) as cm3, \
make_context_manager(4) as cm4 \
:
... # backslashes and an ugly stranded colon The stranded colon is necessary to avoid confusing continuation lines |
Perhaps it's worth also suggesting the use of ExitStack in the documentation? |
This may happen in 3.10! Per Guido:
|
According to https://docs.python.org/3.9/whatsnew/3.9.html#pep-617-new-parser and https://bugs.python.org/issue12782#msg372385 multi-line Python 3.9.0b4 (default, Jul 4 2020, 17:09:40)
[Clang 11.0.3 (clang-1103.0.32.62)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> from contextlib import contextmanager
>>> @contextmanager
... def ctx(name):
... print(f"Hi from {name}!")
... yield
... print(f"Bye from {name}!")
...
>>> with (
... ctx("Alice"),
... ctx("Bob")
... ):
... print("Happy clouds!")
...
Hi from Alice!
Hi from Bob!
Happy clouds!
Bye from Bob!
Bye from Alice! |
I'm just curious, where are things on this? I downloaded the latest black on pip (19.10b0) and I still seem the same issues on with statements. I'm actually waiting on this to be resolved in order to adopt black :-). |
I've also stumbled into this - it's somewhat confusing to have a section in docs/the_black_code_style.md explicitly saying 'we will now do X' and then not do that. Running 20.8b1 will reformat:
into:
This matches the comments above but as mentioned doesn't match the current content of the style document:
Perhaps this could be updated to say "So Black will in future ..." (or words to that effect) rather than "So Black will now ..." ? |
I think this is waiting on the PEG parser support to do parenthesised with |
As of 3.9, it's unofficially supported, but if you enable the Once they drop the old parser entirely in 3.10 it will be official. Perhaps the new formatting with (
ctx("Alice"),
ctx("Bob")
):
print("Happy clouds!") can be part of a 3.10 language mode which you could force if you're using 3.9 and are willing to commit to the new parser. I'm just not sure whether there are other grammar changes that 3.10 introduces... |
Whilst ideally just shifting everything to 3.9 (or 3.10, or ...) and using the new parser which supports this is a perfectly valid solution, it doesn't really help anyone who cannot make that shift. It might be reasonable then update the documentation to match what Black currently does (with an explanation of why?) and that when 3.10 support is added the intent is to behave a little nicer. I attempted to work through the current implementation to see if there was some minimal adjustment which could improve the behaviour here but didn't spend enough time to be able to say one way or the other. I did put together a (failing) test with a rough pass at a before/after comparison (using [ (I spent some time poking fairly randomly at this and made a lovely mess which obviously doesn't work. I don't know enough about all the interactions to be able to diagnose what I've done wrong. I suspect this is made more complex by the |
I realize this isn't very relevant to the context of re-formatting, but in case others fighting this and wishing for a practical solution... After fighting this for too long, I decided to ask the internets whether I could make a helper to clean up the usage of import foo_really_really_really_long_module_name
from contextlib import contextmanager, ExitStack
@contextmanager
def contexts(cms):
with ExitStack() as stack:
yield [stack.enter_context(cm) for cm in cms]
with contexts(
[
patch("foo_really_really_really_long_module_name.bar", MagicMock()),
patch("foo_really_really_really_long_module_name.baz", MagicMock()),
]
) as (mock_foo_bar, mock_foo_baz):
... IMO, a short helper is a far better solution until 3.10 support comes around... but since I'm a newb I've probably missed some important problem with this... |
@bryceschober bear in mind you've reimplemented the deprecated https://docs.python.org/2/library/contextlib.html#contextlib.nested You should read the quirks and warnings in the documentation about why this strategy isn't correct
3.9 is already released with support for parenthesised with |
The worst part of this issue is the fact that |
@bersbersbers work has started here #2318 |
is this still open? can I implement what @ambv suggested? |
Status update: thanks to PR #3489 (thanks @yilei!), Black will now use parentheses to break long with statements if it can. Łukasz's suggestion to use backslashes (when the target-version doesn't let Black use parentheses) has yet to be implemented. While this issue hasn't been fully resolved yet, there is no point in keeping two issues open at once about the same issue. So, I'm closing this issue in favour of #3484 which tracks the implementation of the backslash-based style. Thanks everyone for participating! |
Closes psf/black#392 Revived from psf/black#558 Relevant to psf/black#664
Long
with
statements are not not broken into several lines:Something like this should be OK, according to PEP8
But black formats its as:
Operating system: macOS 10.14.2
Python version: 3.7.2
Black version: 18.9b0
Does also happen on master: Yes
The text was updated successfully, but these errors were encountered: