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

Reduce indentation for multi-line lists in function calls #2071

Closed
YodaEmbedding opened this issue Mar 28, 2021 · 6 comments
Closed

Reduce indentation for multi-line lists in function calls #2071

YodaEmbedding opened this issue Mar 28, 2021 · 6 comments
Labels
R: duplicate This issue or pull request already exists T: style What do we want Blackened code to look like?

Comments

@YodaEmbedding
Copy link

YodaEmbedding commented Mar 28, 2021

Describe the style change

The square brackets of multi-line lists that are arguments of a function call should appear on the same lines as the function call's surrounding parentheses.

Examples in the current Black style

x = np.array(
    [
        [0, 1, 2, 3, 4],
        [5, 6, 7, 8, 9],
    ]
)

x = np.array(
    [
        [0, 1, 2, 3, 4],
        [5, 6, 7, 8, 9],
    ],
    dtype=np.float32,
)

Desired style

x = np.array([
    [0, 1, 2, 3, 4],
    [5, 6, 7, 8, 9],
])

x = np.array([
    [0, 1, 2, 3, 4],
    [5, 6, 7, 8, 9],
], dtype=np.float32)

Additional context

This is similar to a style often seen in Typescript and similar languages. It is also an elegant style for numpy array declarations, as shown above. In fact, it seems that many other numpy users have also been troubled by the way that black currently formats numpy arrays. For instance, the following Stack Overflow question has been viewed 300 times:
https://stackoverflow.com/questions/61021465/formatting-numpy-arrays-with-black/365102

This rule should of course not be followed in cases where the non-list arguments exceed line length or when there is more than one multi-line list as an argument. In such cases, the current black style is much more reasonable.

@YodaEmbedding YodaEmbedding added the T: style What do we want Blackened code to look like? label Mar 28, 2021
@divtiply
Copy link

divtiply commented May 23, 2021

Another case with dicts:

Current style

d.update(
    {
        'foo': 1,
        'bar': 2,
    }
)

Desired style

d.update({
    'foo': 1,
    'bar': 2,
})

@cooperlees
Copy link
Collaborator

This will be a large change to our current style, so is probably unlikely to be accepted or worked on. It is worth having a discussion on tho.

I personally feel "exploding" the objects more makes it easier to see the types - e.g. here, the lists of lists and the dictionary are way more prominent and clear (i.e. harder to mis-read) with black's current style.

@felix-hilden
Copy link
Collaborator

felix-hilden commented May 24, 2021

Personally I'd also like this. And maybe there's a middle ground: removing the extra indentation becomes messier when other arguments are involved.

# This I like
np.array([
    [0, 1, 2, 3, 4],
    [5, 6, 7, 8, 9],
])

# Waste of space in my opinion
np.array(
    [
        [0, 1, 2, 3, 4],
        [5, 6, 7, 8, 9],
    ]
)

# Fine, but a bit weird
np.array([
    [0, 1, 2, 3, 4],
    [5, 6, 7, 8, 9],
], dtype=np.float32)

# Don't feel strongly about this, but would be more readable
np.array(
    [
        [0, 1, 2, 3, 4],
        [5, 6, 7, 8, 9],
    ],
    dtype=np.float32
)

# Quite ugly
np.array([
    [0, 1, 2, 3, 4],
    [5, 6, 7, 8, 9],
], dtype=np.float32, copy=False, order="F")

# Better and consistent with current style
np.array(
    [
        [0, 1, 2, 3, 4],
        [5, 6, 7, 8, 9],
    ],
    dtype=np.float32,
    copy=False,
    order="F"
)

So perhaps this rule could be applied only when the imploded item is the only one on that level. What do you think?

@ichard26
Copy link
Collaborator

Some more context and discussion from #1339 (which I closed in favour of this issue)

Current format:

frozenset(                                                                              
    {                                                                                                                                                                    
        0,                                                                              
        1,                                                                              
        2,                                                                              
        3,                                                                              
        ...,   # (continues in sequence, omitted for brevity)
        23,                                                                             
    }                                                                                   
)                                                                                       

Proposed format:

frozenset({                                                                              
    0,                                                                                   
    1,                                                                                   
    2,                                                                                   
    3,                                                                                   
    ...,                                                                                 
    23,                                                                                                                                                                  
})

This is a very common construct. The distinct bracket types provide enough differentiation between the inner and the outer expression; the extra level of indentation is just noise.

Looks tangentially-related: #177

#1339 (comment)

Also seems worth noting that in practice the expressions on each line are often longer than in this example (numbers from 0 to 23), so saving the extra level of indentation here can end up eliminating the need to wrap the inner expressions and other cascading effects that ultimately make it harder to read code that black is currently producing.

Is this an issue that a new contributor might be able to fix (potentially with some pointers on how to get started)? Thanks:)

#1339 (comment)

@ichard26 ichard26 added the R: duplicate This issue or pull request already exists label May 30, 2021
@ichard26
Copy link
Collaborator

Actually just found #1811... I'm closing this in favour of that now :)

@stephane-2
Copy link

Since #1865 got closed in favor of this issue, this issue's scope is now broadened to also cover nested function calls (where the outer function called only takes one argument):

foo(bar(
    <long argument list>
))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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

6 participants