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

A single conditional expression inside a list is unnecessarily parenthesized #3545

Open
yilei opened this issue Feb 4, 2023 · 5 comments · May be fixed by #4312
Open

A single conditional expression inside a list is unnecessarily parenthesized #3545

yilei opened this issue Feb 4, 2023 · 5 comments · May be fixed by #4312
Labels
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

@yilei
Copy link
Contributor

yilei commented Feb 4, 2023

Describe the style change

Examples in the current Black future style

items = [
    (
        {"key1": "val1", "key2": "val2", "key3": "val3"}
        if some_var == "longstring"
        else {"key": "val"}
    )
]

Desired style

items = [
    {'key1': 'val1', 'key2': 'val2', 'key3': 'val3'}
    if some_var == 'longstring'
    else {'key': 'val'}
]

Additional context

This is a change from #2278.

I think for this edge case, it shouldn't be parenthesized which also adds an extra indentation level.

Playground link: https://black.vercel.app/?version=stable&state=_Td6WFoAAATm1rRGAgAhARYAAAB0L-Wj4ADgAI9dAD2IimZxl1N_Wlw1a7WWyFbt4o765uThnUap6YXDKg-y2hZjBQk1tHkIJUmRf3ubZW6qh6rwYiTz123BJex-bSSddR0SO6n6vZGd9BpWqXFwR2_6FGijjM_Dne5AzdiE1FlCq8_2Dsx5LlvLchM8KOQsFr8WMFH9R1i7Ks2Es0D6bZmm7Cl6fnGsY_a6uEkAAACpTN_YLIf7gwABqwHhAQAAKlDv4rHEZ_sCAAAAAARZWg==

@yilei yilei added the T: style What do we want Blackened code to look like? label Feb 4, 2023
@ichard26 ichard26 added 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 labels Feb 5, 2023
@ichard26
Copy link
Collaborator

ichard26 commented Feb 5, 2023

I agree.

@Utkarshn10
Copy link

Why are we adding a paranthesis pair, in which case would it be useful?

@mariuswallraff
Copy link

I got a related case which does not use a list:

first (shorter) case:

black output for 24.2.0:

some_long_variable_name = True

abc = round(
    (
        1234.5678
        if some_long_variable_name
        else 12345.678 if not some_long_variable_name else 123456.78
    ),
    1,
)

I'd expect and prefer this:

some_long_variable_name = True

abc = round(
    1234.5678
    if some_long_variable_name
    else 12345.678 if not some_long_variable_name else 123456.78,
    1,
)

second case:

black output for 24.2.0 (with line length 99):

some_very_very_very_veeeeeeeeeeeeery_long_condition_to_check = True

abc = round(
    (
        1234.5678
        if some_very_very_very_veeeeeeeeeeeeery_long_condition_to_check
        else (
            12345.678
            if not some_very_very_very_veeeeeeeeeeeeery_long_condition_to_check
            else 123456.78
        )
    ),
    1,
)

I'd expect and prefer this:

some_very_very_very_veeeeeeeeeeeeery_long_condition_to_check = True

abc = round(
    1234.5678
    if some_very_very_very_veeeeeeeeeeeeery_long_condition_to_check
    else (
        12345.678
        if not some_very_very_very_veeeeeeeeeeeeery_long_condition_to_check
        else 123456.78
    ),
    1,
)

@cobaltt7 cobaltt7 linked a pull request Apr 17, 2024 that will close this issue
cobaltt7 added a commit to cobaltt7/black that referenced this issue Apr 17, 2024
@cobaltt7
Copy link
Contributor

@yilei I've opened #4312 that fixes this issue. I added it under the --unstable style (at least for now) due to #4036.

It's also worth noting that the current --unstable style already formats your example as:

items = [(
    {'key1': 'val1', 'key2': 'val2', 'key3': 'val3'}
    if some_var == 'longstring'
    else {'key': 'val'}
)]

which is better than the stable style, but could still be improved.

I took a look at the examples @mariuswallraff provided, but I believe that change was the intention of #2278 in the first place, based off the test cases it adds.

@mariuswallraff
Copy link

Thank you for the fix and for having a look at my post. After reading the other pull request and issue again, I have to agree with you, that change was intentional for the cases I posted. I'll have to get used to the extra indent. 😄

JelleZijlstra added a commit to cobaltt7/black that referenced this issue Apr 24, 2024
cobaltt7 added a commit to cobaltt7/black that referenced this issue Jun 7, 2024
cobaltt7 added a commit to cobaltt7/black that referenced this issue Oct 6, 2024
cobaltt7 added a commit to cobaltt7/black that referenced this issue Oct 8, 2024
cobaltt7 added a commit to cobaltt7/black that referenced this issue Nov 4, 2024
cobaltt7 added a commit to cobaltt7/black that referenced this issue Nov 24, 2024
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. 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

Successfully merging a pull request may close this issue.

6 participants
@yilei @Utkarshn10 @cobaltt7 @ichard26 @mariuswallraff and others