Skip to content

Style guide fix for W503 W504#1382

Merged
LefterisJP merged 3 commits intoraiden-network:masterfrom
LefterisJP:style_guide_fix_W503_W504
Apr 25, 2018
Merged

Style guide fix for W503 W504#1382
LefterisJP merged 3 commits intoraiden-network:masterfrom
LefterisJP:style_guide_fix_W503_W504

Conversation

@LefterisJP
Copy link
Contributor

@LefterisJP LefterisJP commented Apr 24, 2018

All of a sudden flake8 started reporting a W504 violation for our code due to us putting the line break after the operator in long expressions with operators that were broken in multiple lines.

This was introduced in the latest version of flake8 and made our code non flake8 compatible right under our noses.
I tried to change it to have the line break before the operator and then flake8 reported a W503 violation. Damned if you do, damned if you don't.

That looked fishy so I did some digging:
PyCQA/pycodestyle#498
PyCQA/pycodestyle#502

The W503 bug is already fixed as it’s added to the default ignore list but you have a ignore option configured in setup.cfg and if that’s the case the default ignores aren’t respected anymore. This is in my opinion pretty stupid behavior but a flake8 thing I can’t do anything about. That means though that if you specify a ignore list you have to list all default ignores as well. So I updated the list in setup.cfg and now the tests pass without problems.

Apparently this flake8 violation was considered opinionated and put in the default ignores. But there is a bug that if you have any ignores specified (which we do) then you lose all the default ignores.

Solution

Put W504to the ignores, so that flake8 reports correctly when we ever make a W503 violation. Add the rule that line breaks should come after the operator to the style guide.

Note: This rule also applies to solidity code.

By having any ignore on setup.cfg both style rules were activated so we need to
choose one. We go with the operator before the newline.
LefterisJP added a commit to loredanacirstea/raiden-contracts that referenced this pull request Apr 24, 2018
@loredanacirstea
Copy link
Contributor

loredanacirstea commented Apr 24, 2018

@LefterisJP , why not just add W503 to setup.cfg and follow PEP 8?

Ah, you would have a lot of violations, therefore a lot of code to change. Ok.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 62.749% when pulling 45fe431 on LefterisJP:style_guide_fix_W503_W504 into 108e0c3 on raiden-network:master.

@coveralls
Copy link

coveralls commented Apr 25, 2018

Coverage Status

Coverage increased (+12.3%) to 74.963% when pulling 45fe431 on LefterisJP:style_guide_fix_W503_W504 into 108e0c3 on raiden-network:master.

@LefterisJP LefterisJP merged commit ab9ba94 into raiden-network:master Apr 25, 2018
@LefterisJP LefterisJP deleted the style_guide_fix_W503_W504 branch April 25, 2018 07:27
@LefterisJP
Copy link
Contributor Author

@loredanacirstea The good thing is that if we ever wanna change the style we just ignore W503 instead and flake8 will show us every violation that we need to address. Hopefully we won't do that soon though.

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.

4 participants