Skip to content

Conversation

@udim
Copy link
Member

@udim udim commented Mar 2, 2018

Also add missing unittest.main() calls.

@udim
Copy link
Member Author

udim commented Mar 2, 2018

R: @chamikaramj

@@ -64,6 +64,7 @@ echo "Running pycodestyle for module $MODULE:"
pycodestyle "$MODULE" --exclude="$FILES_TO_IGNORE"
echo "Running flake8 for module $MODULE:"
flake8 $MODULE --count --select=E999 --show-source --statistics
Copy link

@cclauss cclauss Mar 2, 2018

Choose a reason for hiding this comment

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

My sense is that E901,E999,F821,F822,F823 are the "showstopper" flake8 issues that can halt the runtime with a SyntaxError, NameError, etc. The other flake8 issues are merely "style violations" -- useful for readability but they do not effect runtime safety.

I would therefore suggest replacing --select=E999 with --select=E901,E999,F822,F823 because the codebase currently passes all of these tests and we would like to avoid any backsliding. We would also want to add F821 (undefined names!) to that list but work must be completed on basestring, buffer(), cmp(), file(), unicode, xrange(), and #4561 before that can be done.

@holdenk

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi, did you mean to make a comment on this PR?

Copy link

Choose a reason for hiding this comment

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

Yes. As long as changes are being made, this is a good one to make as well. Just want to avoid backsliding.

Copy link
Contributor

@chamikaramj chamikaramj left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks.

@chamikaramj chamikaramj merged commit 1eb5f09 into apache:master Mar 5, 2018
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.

3 participants