pre-commit configured and executed #434
Conversation
.flake8
Outdated
| # E402: module level import not at top of file | ||
| # I100: Import statements are in the wrong order | ||
| # I101: Imported names are in the wrong order. Should be | ||
| ignore = E, C, W, F401, F403, F811, F841, E402, I100, I101, D400 |
There was a problem hiding this comment.
Was this change necessary to deal with the black autoformatter? It seems like an unexpectedly long list of exclusions.
In general I think we should be as strict as possible except where it conflicts with the black formatter.
There was a problem hiding this comment.
Actually maybe this is fine, is it to with removing select = F?
There was a problem hiding this comment.
I don't know, i just copy pasted to mimic jupyterhub repo.
I recall Min being opinionated towards being relaxed unless it can cause problems in the flake8 checks. I'm personally inclined towards not needing to put in work to please flake8 unless it is something that can cause a bug.
Perhaps with black, some of these settings are automatically enforced though.
There was a problem hiding this comment.
ignore = E, C, W, F401, F841 seems to pass flake8
Of the remainder:
# F403: import *
Best to avoid in new code as it pollutes the namespace
# F811: redefinition of unused `name` from line `N`
Potential bug in new code, you've assigned a value to a variable and then assigned another value to that variable without using the first value
# E402: module level import not at top of file
# I100: Import statements are in the wrong order
# I101: Imported names are in the wrong order. Should be
These are presumably taken care of by id: reorder-python-imports?
There was a problem hiding this comment.
consideRatio#1 cleans up the unused imports
There was a problem hiding this comment.
Thanks! Makes sense to me!!
There was a problem hiding this comment.
It can be a lot of work to make an old repo compliant with flake8, I suspect this is one reason for lots of exclusions. In general once the code is compliant it's not too difficult to keep it that way, I've had several bugs that were detected by flake8 before I'd even run my tests.
Arguably for a repo like oauthenticator where we don't run a lot of the code ourselves we should be even stricter as we're often merging solely based on code review, but that's a separate discussion 😄 . Shall we merge this PR?
|
A few minor comments, but as you can probably guess I'm in favour of pre-commit 😄 |
Co-authored-by: Simon Li <orpheus+devel@gmail.com>
Remove unused imports, reduce flake8 exclusions list
|
❤️ thank you soo much @manics for your thorough care and effort, i love it! |
General maintenance to follow the common practice of having a pre-commit configuration setup for the JupyterHub org repo.