Multiline strings cause unnecessary optional parentheses

[bitprophet]

Background

• macOS 10.12
• Python 3.6.4
• 18.5b0 (vs 18.4a4)

This issue springs off a short twitter convo I had with @ambv, current end of thread is here: https://twitter.com/llanga/status/997721115530219520

Reported therein is the first example of this issue and I've since found a different one, so figured it was time to make a formal ticket.

(N.B. that thread mentions # noqa comments but I've determined this is not truly relevant/useful, especially given the 2nd example below which seems not a line length problem.)

Long string literals

Long string literal assignment values get indented and wrapped in parenthesis, despite this not getting the line under the desired line-length.

Example was this original pre-blacken line (at a relatively deep indent level):

err = "Refusing to be ambiguous: connect() kwarg '{}' was given both via regular arg and via connect_kwargs!"

Black 18.4a4 leaves it alone; 18.5b0 turns it into:

err = (                                                                                                    
    "Refusing to be ambiguous: connect() kwarg '{}' was given both via regular arg and via connect_kwargs!"
)                                                                                                          

if x in y where y is a multiline string

This is actually just after the above example and at same indent level - but in this case, the line was not previously longer than any defined limits so line length should not (AFAIK) have triggered (though I am thinking perhaps black is condensing it in one pass, causing it to become too long, then parenthesizing the result in a second pass? then again, the string here is multiline and thus from perspective of the opening line would only be 3 chars long?)

Original pre-black:

for key in """                           
    hostname                             
    port                                 
    username                             
""".split():                             
    if key in self.connect_kwargs:       
        raise ValueError(err.format(key))

As before, black 18.4a4 makes no changes, but 18.5b0 turns it into:

for (                                    
    key                                  
) in (                                   
    """                                  
    hostname                             
    port                                 
    username                             
""".split()                              
):                                       
    if key in self.connect_kwargs:       
        raise ValueError(err.format(key))

You can see that both the x and y clauses in the for x in y expression have become needlessly parenthesized. (The body of the for is untouched and just there for clarity.)

[ambv]

Thanks for your report, I will handle this for the next release.

[bitprophet]

Found another even less tractable instance of this, which is admittedly rare: super long non string literals like this hilarious GSSAPI related hex value: https://github.com/paramiko/paramiko/blob/413bf37df7c9bf88d70d2b046935687c072980d4/paramiko/kex_gss.py#L68

As with long string literals, it gets indented inside enclosing parentheses and the # noqa put after the closing paren, where flake8 can't see it. (Also seems to add additional whitespace, versus what is done to strings, but not sure that matters.) Diff example (there are another 2-3 in this file that are same behavior):

diff --git a/paramiko/kex_gss.py b/paramiko/kex_gss.py
index 5eaaa5d..fac9b11 100644
--- a/paramiko/kex_gss.py
+++ b/paramiko/kex_gss.py
@@ -77,7 +73,9 @@ class KexGSSGroup1(object):
     4462 Section 2 <https://tools.ietf.org/html/rfc4462.html#section-2>`_
     """
     # draft-ietf-secsh-transport-09.txt, page 17
-    P = 0xFFFFFFFFFFFFFFFFC90FDAA22168C234C4C6628B80DC1CD129024E088A67CC74020BBEA63B139B22514A08798E3404DDEF9519B3CD3A431B302B0A6DF25F14374FE1356D6D51C245E485B576625E7EC6F44C42E9A637ED6B0BFF5CB6F406B7EDEE386BFB5A899FA5AE9F24117C4B1FE649286651ECE65381FFFFFFFFFFFFFFFF  # noqa
+    P = (
+        0xFFFFFFFFFFFFFFFFC90FDAA22168C234C4C6628B80DC1CD129024E088A67CC74020BBEA63B139B22514A08798E3404DDEF9519B3CD3A431B302B0A6DF25F14374FE1356D6D51C245E485B576625E7EC6F44C42E9A637ED6B0BFF5CB6F406B7EDEE386BFB5A899FA5AE9F24117C4B1FE649286651ECE65381FFFFFFFFFFFFFFFF
+    )  # noqa
     G = 2
     b7fffffffffffffff = byte_chr(0x7f) + max_byte * 7  # noqa
     b0000000000000000 = zero_byte * 8  # noqa

---

Above is a report for the maintainers, below are my own personal thoughts on how to mitigate this in a "black + flake8" using project.

I've been sucking it up and breaking strings apart in one fashion or another (implicit concat, sub-variable assignments, etc) but that's obviously not gonna work here. Can't think of anything off the top of my head besides structural changes:

• flake8 can't (yet) ignore ranges of lines so that's out
• move these constants to a separate file and use file-wide # flake8: noqa (not great, but not awful, though it's a "bigger" change than I'd like to do in a formatting-only bugfix patchset)
• Exclude line length check in flake8 entirely (not ideal as I'm not sure what else black currently is intended to skip over, or which it might incidentally skip over; but if I'm otherwise relegating line length checks to black, that is probably paranoia)
• go back to using black 18.4a4 for Paramiko specifically (I don't like inconsistency and my other projects are so far all on 18.5b0)

[carljm]

Isn't the simplest workaround here to just manually move the # noqa comment back up to the same line as the long constant? Then flake8 will respect it again, and Black won't move it.

[bitprophet]

I could swear Black did move that around when I tried that last time, but I'll double check...

[bitprophet]

Yea, nope, I'm wrong! Thanks for the idiot check 😁 That's way simpler.

[carljm]

To be clear, that's a workaround for unfortunate behavior: ideally Black wouldn't add the useless wrapping parens here, and probably also ideally Black would try harder not to move # noqa and # type: ignore comments (though the latter is pretty hard in some cases, since it can't know which part of a multi-expression long line the comment was originally needed for.)

[bitprophet]

Totally, it doesn't obviate this overall ticket, but it does help me with that new corner case. Appreciated!

[ambv]

@bitprophet, I totally agree with you that cases where content of optional parentheses isn't itself splittable, it doesn't make any sense to actually wrap it in those optional parentheses. That's a bug and we'll fix it. It's tracked in #273.