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

False negative with statement ending with parens #42

Closed
asottile opened this issue Jan 5, 2018 · 14 comments
Closed

False negative with statement ending with parens #42

asottile opened this issue Jan 5, 2018 · 14 comments

Comments

@asottile
Copy link
Owner

asottile commented Jan 5, 2018

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
Copy link

smagafurov commented Oct 18, 2018

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
Copy link
Owner Author

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

@smagafurov
Copy link

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

@asottile
Copy link
Owner Author

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

@smagafurov
Copy link

sadly...

@asottile
Copy link
Owner Author

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
Copy link

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
Copy link
Owner Author

asottile commented Oct 18, 2018

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

I don't know what to tell you

@smagafurov
Copy link

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
Copy link
Owner Author

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

@smagafurov
Copy link

smagafurov commented Oct 18, 2018

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
Copy link
Owner Author

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

@smagafurov
Copy link

smagafurov commented Oct 18, 2018

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

Please stop

ok

Repository owner locked and limited conversation to collaborators Oct 18, 2018
Repository owner unlocked this conversation Oct 18, 2018
@smagafurov
Copy link

smagafurov commented Oct 18, 2018

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants