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

Long with statements are not broken into several lines #664

Closed
mhham opened this issue Jan 9, 2019 · 26 comments
Closed

Long with statements are not broken into several lines #664

mhham opened this issue Jan 9, 2019 · 26 comments
Labels
C: target version Related to --target-version, e.g. autodetection F: linetoolong Black makes our lines too long F: parentheses Too many parentheses, not enough parentheses, and so on. S: accepted The changes in this design / enhancement issue have been accepted and can be implemented T: style What do we want Blackened code to look like?

Comments

@mhham
Copy link

mhham commented Jan 9, 2019

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 mhham changed the title Long with statements are not broken Long with statements are not broken into several lines Jan 9, 2019
@JelleZijlstra
Copy link
Collaborator

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

@mhham
Copy link
Author

mhham commented Jan 10, 2019

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

@DanCardin
Copy link

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.

@colecrtr
Copy link

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.

@zsol zsol added R: not a bug This is deliberate behavior of Black. F: parentheses Too many parentheses, not enough parentheses, and so on. labels Feb 14, 2019
@advance512
Copy link

+1 for backslashes in this context

@rogalski
Copy link

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

@aklajnert
Copy link

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.

@graingert
Copy link
Contributor

Imho most of these would look nicer with contextlib.ExitStack

@aarchiba
Copy link

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

@mhham
Copy link
Author

mhham commented Dec 11, 2019

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

@ambv
Copy link
Collaborator

ambv commented Mar 3, 2020

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 ambv added stable and removed R: not a bug This is deliberate behavior of Black. F: parentheses Too many parentheses, not enough parentheses, and so on. labels Mar 3, 2020
@ambv
Copy link
Collaborator

ambv commented Mar 4, 2020

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.

@graingert
Copy link
Contributor

graingert commented Mar 4, 2020

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

@terrdavis
Copy link

This may happen in 3.10!

Per Guido:

If we introduce a PEG-based parser, we can do this without hacking the tokenizer. See we-like-parsers/pegen_experiments#229

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

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

@jmehnle
Copy link

jmehnle commented Jul 14, 2020

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!

@quicknir
Copy link

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

@tom-saunders
Copy link
Contributor

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

@graingert
Copy link
Contributor

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

@terrdavis
Copy link

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

@tom-saunders
Copy link
Contributor

tom-saunders commented Sep 16, 2020

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

@bryceschober
Copy link

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

@graingert
Copy link
Contributor

graingert commented Nov 18, 2020

@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

@bersbersbers
Copy link

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.

@graingert
Copy link
Contributor

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 #2318

@ichard26 ichard26 added T: style What do we want Blackened code to look like? F: parentheses Too many parentheses, not enough parentheses, and so on. S: accepted The changes in this design / enhancement issue have been accepted and can be implemented C: target version Related to --target-version, e.g. autodetection labels Mar 26, 2022
@alongouldman
Copy link

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

@ichard26
Copy link
Collaborator

ichard26 commented Feb 1, 2023

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: target version Related to --target-version, e.g. autodetection F: linetoolong Black makes our lines too long F: parentheses Too many parentheses, not enough parentheses, and so on. S: accepted The changes in this design / enhancement issue have been accepted and can be implemented T: style What do we want Blackened code to look like?
Projects
None yet
Development

No branches or pull requests