feat: Configure and run pre-commit hooks #6
Conversation
Allow ruff to discover all files, instead of hardcoding every path which can be error prone, e.g. including "tests/" in the list meant that nested Python files were not being linted, while other paths were stale. There is no need for this list since we truly want to lint all Python files, which ruff does by default. Signed-off-by: Phoevos Kalemkeris <[email protected]>
Drop the explicit ruff.format configuration from the pyproject.toml file since it corresponds to ruff's default values, making it harder for someone familiar with the tool to tell deviations from the default behaviour. Given that ruff strives to maintain compatibility with black, these defaults are unlikely to change. Signed-off-by: Phoevos Kalemkeris <[email protected]>
Drop configuration settings that match ruff's default values (also see previous commit for the changes in ruff.format). Signed-off-by: Phoevos Kalemkeris <[email protected]>
Drop E501 and E226 from ruff.lint's ignore list: * E226 does not appear in the codebase therefore there's no reason to include it there * Given that our line-length limit is already high, we shouldn't ignore E501 Ignoring C901 is not a good idea; the only reason I'm leaving it in is because it would require a lot of refactoring to fix it. We should address this in the future. Signed-off-by: Phoevos Kalemkeris <[email protected]>
Signed-off-by: Phoevos Kalemkeris <[email protected]>
Imports are highly inconsistent throughout the project, using a mix of absolute as well as explicit and implicit relative imports. For this reason, we have to explicitly specify the known first-party and local folders for isort to work correctly. Signed-off-by: Phoevos Kalemkeris <[email protected]>
I strongly believe that the limit should be reduced, but feel free to drop this commit if you disagree. Signed-off-by: Phoevos Kalemkeris <[email protected]>
Run 'ruff check --fix' followed by 'ruff format' and resolve remaining errors manually. For the most part, the manual changes involved one of the 3 following actions: * Split long strings into multiple lines wrapped with parentheses * Refactor code, e.g. retrieve dict values and evaluate conditionals before using the results in f-strings * Move long strings to files (this can be done in many more places around the codebase) Signed-off-by: Phoevos Kalemkeris <[email protected]>
Set up pre-commit hooks with the pre-commit Python package, enabling the following, as well as running them manually for the first time on the entire repository with 'pre-commit run --all-files': - trailing-whitespace - end-of-file-fixer - check-yaml - check-added-large-files - check-json - check-toml - ruff - ruff-format Signed-off-by: Phoevos Kalemkeris <[email protected]>
|
@baixiac, as stated above this PR is rebased on top of the one setting up and running ruff formatting. It's only the last commit that's new, which should be straightforward enough to review. |
|
WRT the failing CI, I see that the Do we still need Python 3.8 compatibility? Is anyone still using it or can we just remove it and work towards newer versions? |
|
So this is also ready for review. Guess @tomolopolis and @kawsarnoor can chime in here as well given it is related to #5. |
|
Hey @baixiac do you still want us to review this? Seems like you want to kick off a wider discussion since you are unhappy with some of suggestions from #5. From my side I think it's best that you and @phoevos maybe iron this out since it's very likely that you two will be predominantly maintaining the repo. |
Set up pre-commit hooks with the pre-commit Python package, enabling the
following, as well as running them manually for the first time on the
entire repository with
pre-commit run --all-files:Instructions on enabling and running the hooks manually are included in
the CONTRIBUTING.md file which we should extend with more information
about setting up the project from development. In fact, that could be the
information found in the current README.md.
Note: This PR is rebased on top of and should be merged after #5