Skip to content
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

Merged
merged 8 commits into from
Nov 4, 2017
Merged

Fix new flake8 tests #2274

merged 8 commits into from
Nov 4, 2017

Conversation

dlstadther
Copy link
Collaborator

@dlstadther dlstadther commented Oct 31, 2017

Description

Update flake8 compliance

Motivation and Context

Flake8 started linting against stricter standards. This patch fixes the new ones.

  • E722 - bare except
  • E741 - ambiguous variable name
  • E742 - ambiguous class definition

Have you tested this? If so, how?

Travis will run.

@daveFNbuck
Copy link
Contributor

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 AttributeError.

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.

@dlstadther dlstadther changed the title Ignore new flake8 tests Fix new flake8 tests Oct 31, 2017
@dlstadther
Copy link
Collaborator Author

@daveFNbuck I've taken your guidance and fixed the current flake8 failures. I didn't spend the time to really determine what the except clauses should be catching except for the docker AttributeError.

@daveFNbuck
Copy link
Contributor

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.

@dlstadther
Copy link
Collaborator Author

@daveFNbuck Good for me to merge?

@daveFNbuck
Copy link
Contributor

LGTM

@dlstadther dlstadther merged commit 56fbbcb into spotify:master Nov 4, 2017
@dlstadther dlstadther deleted the flake8-update branch November 4, 2017 18:27
This was referenced Jun 29, 2022
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.

2 participants