black icon indicating copy to clipboard operation
black copied to clipboard

A single conditional expression inside a list is unnecessarily parenthesized

Open yilei opened this issue 2 years ago • 5 comments

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 https://github.com/psf/black/pull/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 avatar Feb 04 '23 00:02 yilei

I agree.

ichard26 avatar Feb 05 '23 02:02 ichard26

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

Utkarshn10 avatar Aug 14 '23 16:08 Utkarshn10

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

mariuswallraff avatar Feb 19 '24 12:02 mariuswallraff

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

cobaltt7 avatar Apr 17 '24 17:04 cobaltt7

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

mariuswallraff avatar Apr 20 '24 22:04 mariuswallraff