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
Add pre-commit hooks for bandit, black, codespell, and flake8 #2270
Conversation
I would recommend pulling together the |
@mzjp2 Your review again please.
|
.github/workflows/lint_python.yml
Outdated
- run: codespell --ignore-words-list=nd,reacher,thist,ths -w | ||
- run: flake8 --ignore=E203,E402,E712,E722,E731,E741,F401,F403,F405,F524,F841,W503 | ||
--max-complexity=30 --max-line-length=456 --show-source --statistics | ||
- run: pip install isort mypy pytest pyupgrade safety | ||
- run: isort --check-only --profile black . || true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you wanna move isort
into pre-commit
as well (it works super nicely for sorting your imports when you run pre-commit
locally)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pytest fails after running isort.
.github/workflows/lint_python.yml
Outdated
- run: codespell --ignore-words-list=nd,reacher,thist,ths -w | ||
- run: flake8 --ignore=E203,E402,E712,E722,E731,E741,F401,F403,F405,F524,F841,W503 | ||
--max-complexity=30 --max-line-length=456 --show-source --statistics | ||
- run: pip install isort mypy pytest pyupgrade safety |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, maybe time to remove pytest
from lint_python
given that we now have a dedicated testing job?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that the README installation instructions do not mention Docker, I was looking to install and run the tests without Docker.
Removed isort, mypy, pytest, puupgrade, and safety |
Are we sure that we want to require this to commit? the normal way of handling this is in tests, with a step to make sure people make sure their code is properly linted and so on. If we do this as a get check it will require users to have a bunch of software configured on their computers, which is surely going to create incident. |
The only thing that a contributor really should install is black for reforming code.
The GitHub Action in this PR behaves in exactly that way. |
Hey @cclauss thanks for making this contribution. Do you have a video demoing the workflow with |
Could you please also resolve the merge conflicts? |
@cclauss can you resolve the conflicts and merge master in so that I can merge this? |
…#2270) * Add pre-commit hooks for bandit, black, codespell, and flake8 * Those pesky - -- characters... * Merge workflows and remove reduddency * Resync
As discussed in #2268 it might be helpful to move four fast lint tests into a .pre-commit-config.yaml to halt git commits that contain errors. Given that some contributors will not have pre-commit installed, this PR also includes a GitHub Action so that contributors and maintainers can see which tests have failed.