Add flake8 checks #2363
Add flake8 checks #2363
Conversation
There was a problem hiding this comment.
I like this in general. I'm using PyCharm so I get some of PEP8 for free, but definitely not all.
One personal issue is that I don't like the 79 char line length limit 😉, I prefer 100 chars. Sure I can adjust, but there's a lot of code that's up to 100 chars wide...
For some reason Travis isn't happy running this right now.
punchagan
force-pushed
the
add-flake8-checks
branch
2 times, most recently
from
bc8e7e7 to
ee58a8c
Compare
Sure. I have some checks in my editor too, and that's how I found #2361. But, I think it is good to have some checks before merging/deploying to avoid trivial bugs.
I am not very fond of this limit either. I've actually disabled this check, when running it on travis (E501). Sorry for not mentioning this in the PR description.
There was a missing import which was fixed in #2361 and Travis is now happy. |
|
Great! So how do we merge this? Ideally we shouldn't have any other outstanding PRs when this is merged as there's a good chance these changes will introduce conflicts. |
|
All the pending PRs are small, except for the DRF update PR. We can either wait for them to be merged, or just go ahead with merging this one, because the conflicts will be pretty small, if any. For the DRF update PR: If you think that would be merged in reasonably soon, this can wait for that PR to be in. Otherwise, I can help rebase that PR onto the new develop branch, after this has been merged. |
|
Yep, you're right. We might as well merge this then. I have researched one of the larger fixes needed in the DRF update branch, it's the endpoint pagination. What's needed there is mostly code removal and some config since DRF 3 includes much better pagination tools, so the custom hack Kasper wrote after the DRF update branch was made can be thrown away in large part. So if you think you can fix the style related changes in that branch it'd be great and shouldn't make things harder on my end. |
|
Let's merge it then! 👍
I can help with this, once that branch is ready for a review. Or whenever you think, this should be done. |
|
Commits need reference to an issue number |
punchagan
force-pushed
the
add-flake8-checks
branch
from
b839760 to
58f014f
Compare
punchagan
force-pushed
the
add-flake8-checks
branch
from
58f014f to
65ebc92
Compare
Used the autoflake tool and ran the following command:
find akvo/ -name "*.py" |grep -v __init__.py|grep -v migration|xargs autoflake -i -r --remove-all-unused-imports
Ran the following command and made some minor manual edits:
find akvo/ -name "*.py" |grep -v __init__.py|grep -v migration|xargs autoflake -i -r --remove-unused-variables
find akvo/ -name "*.py" |grep -v __init__.py|grep -v migration|xargs \
autopep8 -i --select E231,E203,E225,E226,W291,E228,E201,E202,W293,E302,E301,W391
find akvo/ -name "*.py" |grep -v __init__.py|grep -v migration|xargs \
autopep8 -i --select \
E221,E241,E251,E271,E272,E115,E122,E123,E124,E125,E126,E127,E128,E231,E203,E225,E226,W291,E228,E201,E202,W293
find akvo/ -name "*.py" |grep -v __init__.py|grep -v migration|xargs autopep8 -i --select E261,E265,E266,E221,E241,E251,E271,E272,E401,E402,E403,E405
find akvo/scripts/ -name "*.py"|xargs autopep8 -i --ignore E501
- The checks will run automatically on Travis. - Getting a 100% coverage will require significantly more effort and work, but having flake8 checks can give us some of the benefits for a relatively low cost. Issues like #2361 can easily be avoided.
punchagan
force-pushed
the
add-flake8-checks
branch
from
65ebc92 to
6e18c49
Compare
work, but having flake8 checks can give us some of the benefits for a
relatively low cost. Issues like Add missing import for _ in rest/viewsets.py #2361 can easily be avoided.
Test plan
This is a fairly big change, but most of the changes were made automatically using
autopep8andautoflake. Each commit tries to stick to one kind of fix, to help review.Closes #2477