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

Should assignments manage optional parentheses automatically? #330

Closed
virtuald opened this issue Jun 9, 2018 · 11 comments
Closed

Should assignments manage optional parentheses automatically? #330

virtuald opened this issue Jun 9, 2018 · 11 comments
Labels
F: parentheses Too many parentheses, not enough parentheses, and so on. R: duplicate This issue or pull request already exists T: style What do we want Blackened code to look like?

Comments

@virtuald
Copy link

virtuald commented Jun 9, 2018

Original:

class Variant:
    pass

class Foo:
    
    @property
    def SupportedMimeTypes(self):
        mimetypes = 'audio/musepack;application/musepack;application/x-ape;' \
            'audio/ape;audio/x-ape;audio/x-musepack;application/x-musepack;' \
            'audio/x-mp3;application/x-id3;audio/mpeg;audio/x-mpeg;' \
            'audio/x-mpeg-3;audio/mpeg3;audio/mp3;audio/x-m4a;audio/mpc;' \
            'audio/x-mpc;audio/mp;audio/x-mp;application/ogg;' \
            'application/x-ogg;audio/vorbis;audio/x-vorbis;audio/ogg;' \
            'audio/x-ogg;audio/x-flac;application/x-flac;audio/flac'
        return Variant('as', mimetypes.split(';'))

Black reformats as:

class Variant:
    pass


class Foo:
    @property
    def SupportedMimeTypes(self):
        mimetypes = "audio/musepack;application/musepack;application/x-ape;" "audio/ape;audio/x-ape;audio/x-musepack;application/x-musepack;" "audio/x-mp3;application/x-id3;audio/mpeg;audio/x-mpeg;" "audio/x-mpeg-3;audio/mpeg3;audio/mp3;audio/x-m4a;audio/mpc;" "audio/x-mpc;audio/mp;audio/x-mp;application/ogg;" "application/x-ogg;audio/vorbis;audio/x-vorbis;audio/ogg;" "audio/x-ogg;audio/x-flac;application/x-flac;audio/flac"
        return Variant("as", mimetypes.split(";"))

Present on master and current release.

@ambv
Copy link
Collaborator

ambv commented Jun 9, 2018

This needs fixing, thanks for the report!

As a workaround for now: wrap the right-hand side of the assignment in parentheses, Black will format this correctly for you and will keep it.

@ambv ambv added T: bug Something isn't working F: parentheses Too many parentheses, not enough parentheses, and so on. labels Jun 9, 2018
@genodeftest
Copy link

This issue also happens when having many variables left of the assignment operator =:

#!/usr/bin/python3

lorem, ipsum, dolor, sit, amet, consectetur, adipiscing, elit, sed, do, \
        eiusmod, tempor, incididunt, ut, labore, et, dolore, magna, aliqua, \
        In, publishing, graphic, design, placeholder, text, commonly, used, \
        demonstrate = (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28)
print(lorem)

becomes

#!/usr/bin/python3

lorem, ipsum, dolor, sit, amet, consectetur, adipiscing, elit, sed, do, eiusmod, tempor, incididunt, ut, labore, et, dolore, magna, aliqua, In, publishing, graphic, design, placeholder, text, commonly, used, demonstrate = (
    1,
    2,
    3,
    4,
    5,
    6,
    7,
    8,
    9,
    10,
    11,
    12,
    13,
    14,
    15,
    16,
    17,
    18,
    19,
    20,
    21,
    22,
    23,
    24,
    25,
    26,
    27,
    28,
)
print(lorem)

I don't know if that is the same bug or not though.

@ambv
Copy link
Collaborator

ambv commented Jun 9, 2018

This issue also happens when having many variables left of the assignment operator =

This won't change.

I don't like fake foo-bar examples because they often hide why you'd do such a thing in real code. If you're really assigning 28 elements of a tuple to 28 names on the left, well, then don't do that.

If you are unpacking a call that returns a 28 element tuple, then you should have a talk with the designer of that function and adopt a named tuple instead. Then you don't have to unpack anything.

If you are unpacking a call that returns a 28 element tuple and there's nothing you can do about this call, then you can do it like this:

lorem, ipsum, dolor, sit, amet, consectetur, adipiscing, elit, sed, *rest = my_call()
do, eiusmod, tempor, incididunt, ut, labore, et, dolore, magna, *rest = rest
aliqua, In, publishing, graphic, design, placeholder, text, commonly, used, demonstrate, *rest = rest

But maybe you actually didn't need all the names anyway. Then just use indexing to pluck the names you needed.

Note on backslashes

Backslashes are one of the two places in the Python grammar that break significant indentation. The other one is multiline strings. You never need backslashes, they are used to force the grammar to accept breaks that would otherwise be parse errors. That makes them confusing to look at and brittle to modify. This is why Black always gets rid of them.

If you're reaching for backslashes, that's a clear signal that you can do better if you slightly refactor your code. I hope some of the examples above show you that there are many ways in which you can do it.

@virtuald
Copy link
Author

virtuald commented Jun 9, 2018

Real Code ™️ of the example posted by genodeftest: https://github.com/exaile/exaile/blob/5734ebb69f03812795e0280dee5262e2c6d5ad0b/plugins/grouptagger/gt_mass.py#L47

The Gtk.Child.widgets thing returns placeholders (I think it's a list, not a tuple?) that are later replaced when the composite widget is instantiated (as a form of dependency injection I suppose). The API is written this way as shorthand instead of typing GtkTemplate.Child.Widget over and over again, and is particularly useful with large numbers of widgets.

How would you design that API without requiring lots of repetitive copy/paste?

(and perhaps that issue belongs in a separate issue)

@ambv
Copy link
Collaborator

ambv commented Jun 9, 2018

Looks like GitHub lost my comment that I put here. So a short version follows:

Before

class GtMassRename(Gtk.Window):
    ...

    found_label,    \
        playlists,      \
        replace,        \
        replace_entry,  \
        search_entry,   \
tracks_list = GtkTemplate.Child.widgets(6)

Suggestion 1

Put as many names as they fit on one line. That changed the expression from 6 lines to two.

class GtMassRename(Gtk.Window):
    ...

    found_label, playlists, replace = GtkTemplate.Child.widgets(6)
    replace_entry, search_entry, tracks_list = GtkTemplate.Child.widgets(6)

Suggestion 2

Alias a popular name that you're using many times in your file. I do that very often.

W = GtkTemplate.Child.Widget

class GtMassRename(Gtk.Window):
    ...

    found_label = W()
    playlists = W()
    replace = W()
    replace_entry = W()
    search_entry = W()
    tracks_list = W()

@genodeftest
Copy link

@ambv sorry for the example code and many thanks for the suggestions!

@ambv ambv changed the title Really long line is produced Should assignments manage optional parentheses automatically? Jun 20, 2018
@ambv ambv added T: style What do we want Blackened code to look like? and removed T: bug Something isn't working labels Jun 20, 2018
@ambv
Copy link
Collaborator

ambv commented Jun 20, 2018

I thought about this some more and the problem here is that Black currently only manages optional parentheses automatically in statements, not in expressions.

I realize that assignments are statements (at least so far). However, there are other assignment-like parts of expressions (like dictionary "key: value" or keyword arguments in function calls). It would feel inconsistent to put extra parentheses in one case but not in those others.

Why not do it everywhere and be done with it? Well:

  • automatic management of those parentheses would mean Black will remove them when it decides that a different style is better; (and sometimes it would be wrong)
  • putting extra parentheses in those places would sometimes cause proliferation of brackets (esp. in the case of function calls and dictionary values), which - again - sometimes would look worse than the current situation.

@ambv
Copy link
Collaborator

ambv commented Jun 20, 2018

Similar problem around await is in #275 and again, the worry is that this is an expression, and would complicate formattings that are simpler today.

@whatisaphone
Copy link

I found a simple case that might be related to this issue.

Input:

aaaa.bbbbbbbbbb["ccccccccccccc"] = "dddddddddddddddddddddddd"

Black's output (black.now.sh, master branch, default settings):

aaaa.bbbbbbbbbb[
    "ccccccccccccc"
] = "dddddddddddddddddddddddd"

What I would expect:

aaaa.bbbbbbbbbb["ccccccccccccc"] = (
    "dddddddddddddddddddddddd"
)

@ivirshup
Copy link

I have another case, close to what was reported in #405, though I have a different preferred resolution. Note that this was indented, in a library with preferred line lengths of 80.

Here's what I initially wrote:

X = np.random.randint(0, 100, (1000, 100)) \
    * np.random.binomial(1, 0.3, (1000, 100))

Which black reformatted to:

X = np.random.randint(0, 100, (1000, 100)) * np.random.binomial(
    1, 0.3, (1000, 100)
)

Which I find hard to read, since a call is being split onto multiple lines. I agree that \ is not great, so I prefer something like:

X = (
    np.random.randint(0, 100, (1000, 100))
    * np.random.binomial(1, 0.3, (1000, 100))
)

However black reformats this the same as the original case. Something that I thought was interesting here is that black actually removes the parenthesis when formatting. I expected black to prefer (or at least not remove) the fluent-like formatting.

Here's an example that reproduces on the black playground:

Example
import numpy as np

def gen_x():
    def inner_x():
        X = (
            np.random.randint(0, 100, (1000, 100))
            * np.random.binomial(1, 0.3, (1000, 100))
        )
        return X

    return X

@ichard26 ichard26 removed the T: user support OP looking for assistance or answers to a question label Apr 28, 2021
@felix-hilden
Copy link
Collaborator

The original code is now formatted with parentheses as:

class Foo:
    @property
    def SupportedMimeTypes(self):
        mimetypes = (
            "audio/musepack;application/musepack;application/x-ape;"
            "audio/ape;audio/x-ape;audio/x-musepack;application/x-musepack;"
            "audio/x-mp3;application/x-id3;audio/mpeg;audio/x-mpeg;"
            "audio/x-mpeg-3;audio/mpeg3;audio/mp3;audio/x-m4a;audio/mpc;"
            "audio/x-mpc;audio/mp;audio/x-mp;application/ogg;"
            "application/x-ogg;audio/vorbis;audio/x-vorbis;audio/ogg;"
            "audio/x-ogg;audio/x-flac;application/x-flac;audio/flac"
        )
        return Variant("as", mimetypes.split(";"))

Also, @ivirshup's case is related to #2156: although the discussion has been about boolean operators only, the parentheses are added if another multiplication is introduced. @whatisaphone's case is a duplicate of #236. So I'll close this issue!

@ichard26 ichard26 added the R: duplicate This issue or pull request already exists label Jun 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F: parentheses Too many parentheses, not enough parentheses, and so on. R: duplicate This issue or pull request already exists T: style What do we want Blackened code to look like?
Projects
None yet
Development

No branches or pull requests

7 participants