black icon indicating copy to clipboard operation
black copied to clipboard

Long with statements are not broken into several lines

Open mhham opened this issue 7 years ago • 25 comments

Long with statements are not not broken into several lines:

Something like this should be OK, according to PEP8

with averylongnameyoucantsplit as youcheater, \
     averylongnameyoucantsplit as youcheater, \
     averylongnameyoucantsplit as youcheater, \
     averylongnameyoucantsplit as youcheater, \
     averylongnameyoucantsplit as youcheater, \
     averylongnameyoucantsplit as youcheater, \
     averylongnameyoucantsplit as youcheater, \
     averylongnameyoucantsplit as youcheater, \
     averylongnameyoucantsplit as youcheater:
    print("hello")

But black formats its as:

with averylongnameyoucantsplit as youcheater, averylongnameyoucantsplit as youcheater, averylongnameyoucantsplit as youcheater, averylongnameyoucantsplit as youcheater, averylongnameyoucantsplit as youcheater, averylongnameyoucantsplit as youcheater, averylongnameyoucantsplit as youcheater, averylongnameyoucantsplit as youcheater, averylongnameyoucantsplit as youcheater:
    print("hello")

Operating system: macOS 10.14.2 Python version: 3.7.2 Black version: 18.9b0 Does also happen on master: Yes

mhham avatar Jan 09 '19 11:01 mhham

I suppose this is Black strongly dislikes backslashes. I'm not sure there's really a great formatting Black can pick here.

JelleZijlstra avatar Jan 09 '19 18:01 JelleZijlstra

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 with statement remains problematic (because of the impossibility to use parentheses as with if). The backslash usage here is exceptional (as mentioned in PEP 8) and brings a real improvement in readability.

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")

mhham avatar Jan 10 '19 12:01 mhham

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 withs like this to the point where it would want to wrap is an antipattern it shouldn't try to handle and make pretty.

DanCardin avatar Jan 29 '19 20:01 DanCardin

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 with statements as pretty as it can because some disagree with the usage of backslashes. As @mhham said:

The backslash usage here is exceptional (as mentioned in PEP 8) and brings a real improvement in readability.

colecrtr avatar Jan 29 '19 20:01 colecrtr

+1 for backslashes in this context

advance512 avatar Apr 23 '19 17:04 advance512

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()

rogalski avatar Jun 21 '19 08:06 rogalski

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.

aklajnert avatar Jul 05 '19 09:07 aklajnert

Imho most of these would look nicer with contextlib.ExitStack

graingert avatar Jul 13 '19 18:07 graingert

This is the same issue as #557 ; see discussion there.

aarchiba avatar Sep 23 '19 12:09 aarchiba

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 with statements inside parentheses, which could take quite some time ...

Here is the corresponding python issue: https://bugs.python.org/issue12782

mhham avatar Dec 11 '19 10:12 mhham

I changed my mind on this. Let's special-case with statements with multiple context managers and use backslashes for those.

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.

ambv avatar Mar 03 '20 11:03 ambv

In the spirit of documentation-driven development, this is a proposed section of the docs explaining this cop-out:

Backslashes and context managers

Black dislikes backslashes and removes them on every occasion. For every grammar rule in Python there is a nicer way to format your expression without resorting to backslashes. Except for one: with statements using multiple context managers. Python's grammar does not allow organizing parentheses around the series of context managers.

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 from the hanging indent of the with statement with the with body block. This formatting will only apply if there is more than one context manager used in a single with statement and they don't fit in a single line.

ambv avatar Mar 04 '20 19:03 ambv

Perhaps it's worth also suggesting the use of ExitStack in the documentation?

graingert avatar Mar 04 '20 20:03 graingert

This may happen in 3.10!

Per Guido:

If we introduce a PEG-based parser, we can do this without hacking the tokenizer. See https://github.com/gvanrossum/pegen/issues/229

I'd propose to aim for Python 3.10 (if the PEG parser happens).

https://bugs.python.org/issue12782#msg363755

terrdavis avatar Mar 16 '20 20:03 terrdavis

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 with will first be supported in Python 3.9, not 3.10. And, lo and behold:

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!

jmehnle avatar Jul 14 '20 22:07 jmehnle

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 :-).

quicknir avatar Aug 13 '20 18:08 quicknir

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:

        with patch("logging.FileHandler") as file_handler, patch("sqlitecaching.config.log") as config_log:

into:

       with patch("logging.FileHandler") as file_handler, patch(
            "sqlitecaching.config.log"
        ) as config_log:

This matches the comments above but as mentioned doesn't match the current content of the style document:

We don't want formatting 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

So 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

Perhaps this could be updated to say "So Black will in future ..." (or words to that effect) rather than "So Black will now ..." ?

tom-saunders avatar Sep 14 '20 22:09 tom-saunders

I think this is waiting on the PEG parser support to do parenthesised with

graingert avatar Sep 14 '20 22:09 graingert

As of 3.9, it's unofficially supported, but if you enable the -X oldparser option, it will not work.

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...

terrdavis avatar Sep 15 '20 04:09 terrdavis

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 [test_long_strings] as the starting point), but I don't really know what/where the next steps would need to be made, or if the output in there is 'correct'.

(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 \\\n character sequence for continuation not being part of the grammar but hand waved away as part of ingest.)

tom-saunders avatar Sep 16 '20 11:09 tom-saunders

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 contextlib.ExitStack(), and it's actually pretty trivial, and even supports capturing the mocks for my use-case, as formatted by black:

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 avatar Nov 18 '20 02:11 bryceschober

@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

until 3.10 support comes around

3.9 is already released with support for parenthesised with

graingert avatar Nov 18 '20 02:11 graingert

The worst part of this issue is the fact that black stops working entirely as it fails to parse code that is running in stable Python 3.9.

bersbersbers avatar Jun 30 '21 06:06 bersbersbers

The worst part of this issue is the fact that black stops working entirely as it fails to parse code that is running in stable Python 3.9.

@bersbersbers work has started here https://github.com/psf/black/issues/2318

graingert avatar Jun 30 '21 07:06 graingert

is this still open? can I implement what @ambv suggested?

alongouldman avatar Oct 14 '22 13:10 alongouldman

Status update: thanks to PR https://github.com/psf/black/pull/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 https://github.com/psf/black/issues/3484 which tracks the implementation of the backslash-based style.

Thanks everyone for participating!

ichard26 avatar Feb 01 '23 22:02 ichard26