-
Notifications
You must be signed in to change notification settings - Fork 441
Configure flake8. Fix flake8 errors where possible. #283
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
Conversation
As planned, the Travis checks are failing, because I added a flake8 run to the travis configuration.
|
control/lti.py:173:30: E721 do not compare types, use 'isinstance()' Using https://stackoverflow.com/questions/152580/whats-the-canonical-way-to-check-for-type-in-python control/modelsimp.py:413:5: F841 local variable 'Ymat' is assigned to but never used Remove it. control/frdata.py:371:13: E722 do not use bare 'except' Add specific exceptions. |
You also did not follow my suggestion to submit 1 PR per py file (or per sub-package at max). |
Since there is only 1 subpackage, I followed that suggestion. I didn't think fixing 25+ individual files and creating pull requests for all of them would be easier to manage. The person reviewing could always review one file at a time. |
Right. I understand what the errors mean. The point was to get suggestions by someone who wrote the code (or, has more commits, or wants to review) before I change it to something else. |
I offered my time and suggestions and you clearly don't respect that from your response. I have nothing more to offer. |
1 similar comment
Would it be better to just configure & run flake8 with the --exit-zero flag? Then the checks won't fail due to flake8, and we can slowly, unit-by-unit, fix the formatting and code. I don't feel happy either with over 30 changed files in one PR and I would not like this to get hurried through. |
This has to be done in a separate Travis CI job to make sure that later no new PEP8 problems creep in. Over SciPy, we created a very fast job that runs |
Fixed most flake8 warnings and errors in control package. Used autopep8 tool to fix most whitespace related errors. Test files are not done at the moment.
Some other fixed issues :
warn()
statementCurrently ignored flake8 warnings are : E402 (import not at top of file) and W605 (invalid escape sequence)
When reviewing the pull request, hiding whitespace changes will make it a lot easier to review all changes.