Enable code linting (static code analysis) via pre-commit + ruff and fix findings #584
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
linting as in static code analysis to improve the overall code quality (this is not about code style!) pre-commit: The pre-commit framework became pretty widely used in recent years, especially in the Python world, where it originates from. It allows combine multiple hooks and has its own dependency management. There are many hooks for different routine checks like linting and code formatting, see also https://pre-commit.com/hooks.html. The hooks are configured in a file called .pre-commit-config.yaml and so don't pollute the requirements*.txt files. pre-commit can be added as git pre-commit hook via running `pre-commit install` or just be executed on cli (or IDE on every file change) via `pre-commit run --all`. Added this also as comments to the config file. With clever selections of hooks a runtime of <5 seconds can be achieved for a good developer experience, when running on every change. The config contains some standard hooks and ruff as Python code linter. ruff: https://github.com/astral-sh/ruff is proposed as Python linter as it is super fast (due to being implemented in Rust) and implements lots of lints/tests from flake8/bandit/pyupgrade/pylint and more and more big projects adopted it already. The elegant thing with ruff is the unified configuration via pyproject.toml, no matter what rule set gets activated or has to be partially ignored. This PR introduces pre-commit as concept and ruff with only the test category "F" (=pyflakes) and "T20" (=flake8-print) enabled to keep the amount of fixes reviewable. An added GH workflow runs it on every PR to check that there are no findings. The category "F" finds lots of things, that are fixed by this PR, like duplicated imports, un-used return-values, un-reachable code, un-used variables, hard to follow star imports etc. One exception are the type annotations in the controller.py, which are non-standard: Decided to add `# noqa` so that ruff ignores those and resolving those can be tried in a followup PR. Further PRs can add more hooks and/or enable more ruff rule categories, if sensible.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
linting as in static code analysis to improve the overall code quality (this is not about code style!)
pre-commit: The pre-commit framework became pretty widely used in recent years, especially in the Python world, where it originates from. It enables combining multiple hooks and has its own dependency management. There are many hooks for different routine checks like linting (and also code generation / formatting), see also https://pre-commit.com/hooks.html. The hooks are configured in a file called .pre-commit-config.yaml and so don't pollute the requirements*.txt files. pre-commit can be added as git pre-commit hook via running
pre-commit installor just be executed on cli (or IDE on every file change) viapre-commit run --all. Added this also as comments to the config file. With clever selections of hooks a runtime of <5 seconds can be achieved for a good developer experience, when running on every change. The config contains some standard hooks and ruff as Python code linter.ruff: https://github.com/astral-sh/ruff is proposed as Python linter as it is super fast (due to being implemented in Rust) and it implements lots of lints/tests from flake8/bandit/pyupgrade/pylint and more and more big projects adopted it already. The elegant thing with ruff is that it proposes fixes for found issues and the unified configuration via pyproject.toml, no matter what rule category gets activated or has to be partially ignored.
This PR introduces pre-commit as concept and ruff with only the test category "F" (=pyflakes) and "T20" (=flake8-print) enabled to keep the amount of fixes reviewable. An added GH workflow runs it on every PR to check that there are no findings.
The category "F" finds lots of things, that are fixed by this PR, like duplicated imports, un-used return-values, un-reachable code, un-used variables, hard to follow star imports etc.
Further PRs can add more hooks and/or enable more ruff rule categories, if sensible.
This pull request is categorized as a:
Checklist
pytestand made sure all unit tests pass before sumbitting the PRIf you modified or added functionality/workflow, did you add new unit tests?
I have tested this PR on the following platforms/os:
Note: Keep your changes limited in scope; if you uncover other issues or improvements along the way, ideally submit those as a separate PR. The more complicated the PR the harder to review, test, and merge.