asottile/add-trailing-comma

False negative with statement ending with parens

asottile opened this issue · 14 comments

asottile commented 
if (foo and
    bar()):
    pass

I expect to be rewritten to

if (
    foo and
    bar()
):
    pass

But I imagine the same code that prevents this from being rewritten is firing:

foo('bar {}'.format(
    'baz',
))

which I don't think should be rewritten.

Need to come up with a more clever heuristic here.

smagafurov commented

One more example

bankrupts = [{
    'id': company.bankrupt_id,
    'type': 'company',
} for company in found_companies]

This shouldn't be rewritten with:

bankrupts = [
    {
        'id': company.bankrupt_id,
        'type': 'company',
    } for company in found_companies
]
asottile commented

Should or shouldn't? The second is consistent with the style.

smagafurov commented

The first is also consistent with the style.
And therefore shouldn't be rewritten.

asottile commented

It is not, the style prescribes no hugging braces (unless on a line with only braces)

smagafurov commented

sadly...

asottile commented

No not sadly, that's how this tool is designed. If you don't like that style, don't use the tool! 😆

This issue is about making more constructs get rewritten in that way

smagafurov commented

This tool is very useful
So it is sadly :)

Because I really find this style useful:

async def test_ambiguous(api_request):
    response = await api_request('some_method', some_param='some-value')
    assert response['error'] == {
        'code': 6001,
        'message': 'Validation error',
        'data': {'errors': [
            {'code': 'ambiguous', 'message': 'Ambiguous'},
        ]}
    }

I don't want to expand this line 'data': {'errors': [ because it has no "semantic value".

asottile commented

Tough! The tool can't possibly determine """semantic value"""

I don't know what to tell you

smagafurov commented

oops...

This case works fine for me

async def test_ambiguous(api_request):
    response = await api_request('some_method', some_param='some-value')
    assert response['error'] == {
        'code': 6001,
        'message': 'Validation error',
        'data': {'errors': [
            {'code': 'ambiguous', 'message': 'Ambiguous'},
        ]}
    }

add-trailing-comma doesn't touch it!

Problem is only with this

bankrupts = [{
    'id': company.bankrupt_id,
    'type': 'company',
} for company in found_companies]
asottile commented

Look, I think you're missing the point. The whole goal of the tool is so you don't argue about style.

smagafurov commented

The tool just shouldn't touch this case

bankrupts = [{
    'id': company.bankrupt_id,
    'type': 'company',
} for company in found_companies]

And also shouldn't touch this case too

bankrupts = [
    {
        'id': company.bankrupt_id,
        'type': 'company',
    } for company in found_companies
]

They are both valid

A person can choose according to "semantic value" or some other "magic" he likes

asottile commented

I'm sorry, the first one is ugly and will continue to be rewritten. Please stop

smagafurov commented

PyCharm Reformat Code works exactly that way i want
https://www.jetbrains.com/help/pycharm/reformatting-source-code.html

Please stop

ok

smagafurov commented

Very sorry for my inattention.
I did not notice that you are the author of the tool.
:)
Sorry one more time...
And thank for the tool!

  • ❤️1