-
Notifications
You must be signed in to change notification settings - Fork 50
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
Flag-day: Auto-format and lint with black, isort, pylint and bandit + add to CI #439
Flag-day: Auto-format and lint with black, isort, pylint and bandit + add to CI #439
Conversation
- Add black, isort, pylint and bandit to test requirements - Update indent in editorconfig to match black (4 spaces) - Add basic pylintrc file (copy-pasted from python-tuf) - gitignore pre-commit config Signed-off-by: Lukas Puehringer <[email protected]>
Used command: `black --line-length=80 --extend-exclude=_vendor .` Signed-off-by: Lukas Puehringer <[email protected]>
Used command: ``` isort --line-length=80 --extend-skip-glob='*/_vendor' \ --profile=black --project=securesystemslib . ``` Signed-off-by: Lukas Puehringer <[email protected]>
Inline-disable all non-error/fatal pylint issues raised by running `pylint -j 0 --rcfile=pylintrc securesystemslib tests`, by adding inline comments a la `"# pylint: disable=<issue>[, ...]"`. This allows running pylint on future PRs without spending much effort on existing code, whose future is uncertain (see secure-systems-lab#270). The patch was created mostly automatically with this script: https://gist.github.com/lukpueh/41026a3a7a594164150faf5afce94774 Unfortunately, both black and isort reformat inline comments in a way that pylint won't consider them anymore. Thus, some manual adjustments after running above script were necessary. https://black.readthedocs.io/en/stable/faq.html#why-does-my-linter-or-typechecker-complain-after-i-format-my-code Signed-off-by: Lukas Puehringer <[email protected]>
Remove outdated original function. Signed-off-by: Lukas Puehringer <[email protected]>
This seems to be a false positive related to unpacking a tuple in a for loop. Signed-off-by: Lukas Puehringer <[email protected]>
Inline-disable low/medium-severity bandit issues raised by running `bandit --recursive securesystemslib --exclude _vendor`, by adding inline comments a la `"# nosec"`. This allows running bandit on future PRs without spending much effort on existing code, whose future is uncertain (see secure-systems-lab#270). Signed-off-by: Lukas Puehringer <[email protected]>
- Add 'lint' tox environment that runs black, isort and bandit, and mypy (moved from its own env). - Run new tox env in ci GitHub Action. Signed-off-by: Lukas Puehringer <[email protected]>
For usage and details see: https://black.readthedocs.io/en/stable/guides/introducing_black_to_your_project.html Signed-off-by: Lukas Puehringer <[email protected]>
7a4c00b
to
b831bd2
Compare
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.
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.
Looks good. Thanks for the review advice that helped a lot.
The only things that looks potentially dangerous are the import order changes: There's always a risk there but I think the ones you are doing seem low risk and reasonable.
Fixes: #243
Description of the changes being introduced by the pull request:
The most important changes of this PR include:
black
andisort
pylint
andbandit
linters for all non-critical issues(note: fixing those issues here would make review quite difficult, and might be wasted effort, given the uncertain future of some of these modules)
I strongly suggest to review commit by commit and consider below review hints:
minimal functional changes; biggest chunk of the diff is a new
pylintrc
, which is copy-pasted from python-tufmassive purely non-functional change; can be reproduced by running the commands in the commit messages at those commits
massive purely non-functional changes, but repetitive and thus easy to review; unfortunately not automatically reproducible, because the patches required many manual adjustments
minimal functional changes; easy to review commit by commit
Please verify and check that the pull request fulfils the following requirements: