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

Fix a bug causing both W503 and W504 to be ignored #525

Merged
merged 1 commit into from Mar 11, 2020

Conversation

novadev94
Copy link
Contributor

This happens when either W503 or W504 is in the ignore list, autopep8 decided to ignore both of them - which it shouldn't.

@coveralls
Copy link

coveralls commented Feb 17, 2020

Coverage Status

Coverage increased (+0.005%) to 98.639% when pulling 7984fe7 on NovaDev94:master into fded37b on hhatto:master.

@hhatto hhatto self-requested a review February 17, 2020 13:22
@hhatto
Copy link
Owner

hhatto commented Feb 20, 2020

Thanks for contribution 👍

What problem does this change solve?
Could you show us specific Python code for autopep8 and the execution image of autopep8?

Also, could you add some test code for this change?
That will help us better understand the intent of the change.


related issue: #456, #466

@novadev94
Copy link
Contributor Author

novadev94 commented Feb 26, 2020

@hhatto Sorry I forgot to include an example

# setup.cfg
[flake8]
ignore = W503
# So autopep8 should format based on W504 (break before paramemter)
# test.py
a_bool = (
    this and
    that
)

With the current autopep8 behavior, it doesn't re-format it into

# test_formatted.py
a_bool = (
    this
    and that
)

Same behavior (doesn't reformat the operator position) can be seen when we ignore W504 (but not W503).

This is caused by the not all(any(conflict in ignores) for conflict in (W503, W504)) forces both codes to be ignored even when one of them already was.

| W503 | W504 | Ignore | Correct |
|:----:|:----:|:------:|:-------:|
|      |      |  Both  |    Y    |
|   x  |      |  Both  |    N    |
|      |   x  |  Both  |    N    |
|   x  |   x  |  Both  |    Y    |

(The last case is not needed since both of them were already ignored) 

I'm using all(not any(...)) to only add those 2 codes to the ignore list when none of them already was.

@novadev94
Copy link
Contributor Author

Unit tests added

@novadev94
Copy link
Contributor Author

@hhatto I've squashed the unit test commit into the main one. Please let me know if you need extra information about the scope of this change.

@hhatto hhatto added this to the 1.5.1 milestone Mar 11, 2020
Copy link
Owner

@hhatto hhatto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Thank you for contribution!!

@hhatto hhatto merged commit ef15dd5 into hhatto:master Mar 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants