psf / black

The uncompromising Python code formatter
https://black.readthedocs.io/en/stable/
MIT License
38.76k stars 2.44k forks source link

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

Open yilei opened 1 year ago

yilei commented 1 year ago

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==

ichard26 commented 1 year ago

I agree.

Utkarshn10 commented 1 year ago

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

mariuswallraff commented 7 months ago

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 commented 6 months ago

@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 commented 5 months ago

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