-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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 new flake8 tests #2274
Fix new flake8 tests #2274
Conversation
Why are these ones excessive? E741 and E742 only catch three test cases where we use l and I as class and variable names. For the bare except, I see a couple we can fix but the rest can just get around it by catching BaseException. There are only 13 instances of bare excepts in the repo, so it's still a quick fix. Note that if we had this enabled, it would have caught the latest docker commit, which has a bare except where it should be catching an The reason your commit fails is that you modified the wrong line. The line you modified only runs flake8 on the docs. The line above is the one that runs it on everything else. |
These new exclusions include: * E722 - bare except * E741 - ambiguous variable name * E742 - ambiguous class definition
fe65903
to
ed29e95
Compare
@daveFNbuck I've taken your guidance and fixed the current flake8 failures. I didn't spend the time to really determine what the |
I think most of them were meant to catch arbitrary exceptions, so that's fine. The redshift one should probably just be removed. No point in catching an exception just to re-raise it. |
@daveFNbuck Good for me to merge? |
LGTM |
Description
Update flake8 compliance
Motivation and Context
Flake8 started linting against stricter standards. This patch fixes the new ones.
Have you tested this? If so, how?
Travis will run.