Adds local pre-commit hook for ruff linting auto fixes #678
Conversation
I've asked that this PR be reviewed as a priority, but nobody has been available to review it recently. My preference is to get this PR merged as soon as possible. |
rozyczko
left a comment
rozyczko
left a comment
There was a problem hiding this comment.
Changes look good, minor questions raised for consideration.
| # Run the linter, applying any available fixes | ||
| - id: ruff-check | ||
| stages: [ pre-commit, pre-push ] | ||
| args: [ --fix-only ] |
There was a problem hiding this comment.
shouldn't we just report the issue, rather than try fixing them?
There was a problem hiding this comment.
The --fix-only flag restricts ruff to checking only for linting errors where it has an automatic fix available. If any are found, they are applied, and the commit is stopped. The developer can then check what has been changed by those automatic fixes, and then proceed with the commit.
There was a problem hiding this comment.
Note that these fixes will be applied in the CI if they are not done locally.
| # Run the linter, applying any available fixes | ||
| - id: ruff-check | ||
| stages: [ pre-commit, pre-push ] | ||
| args: [ --fix-only ] |
There was a problem hiding this comment.
If we ARE fixing then maybe it makes sense to add formatting as well?
- id: ruff-format
stages: [ pre-commit, pre-push ]
There was a problem hiding this comment.
Given the current state of the code, adding formatting would result in a large number of changes, substantially increasing the burden for developers and PR reviewers. I am keen to introduce formatting at a later stage but this will require work done to go through the codebase, and applying formatting to what is there already.
build_tools/requirements.txt
Outdated
| columnize | ||
| matplotlib | ||
| numpy | ||
| pre-commit |
There was a problem hiding this comment.
shouldn't we also add this dependency in pyproject.toml?
There was a problem hiding this comment.
A couple of comments on this one:
- while it was once murky, PEP517 (and friends) now give very clear separation between:
- "build-system requires" (build-dependencies, what is required to build the wheel),
- run-time dependencies (what is needed to import and use the module), and
- tools that are useful for the developer but unneeded at build-time or run-time.
- tools like
pytest,ruffare in that 3rd category for developer tools: they are neither required for the build process nor at runtime. - as part of that separation,
- build-dependencies must be in the
pyproject.toml[build-system]requiresentry. - runtime-dependencies must be in the
pyproject.toml[project]dependencies, but that can be broken out (as sasmodels does) as a dynamic field into thebuild_tools/requirements.txtfile (and some other files in the same directory for optional runtime dependencies for additional features). - other tools useful for developers don't have a defined place, but
requirements-dev.txtis common (and it could reference the other files to include them; note thathatch-requirements-txtdoesn't follow-rdirectives, butrequirements-dev.txtis never used by hatch and only viapip).
- build-dependencies must be in the
So...
pre-commitis not a requirement to build the wheel: it's not needed forpython -m buildbut is a local tool that helps developers; it shouldn't be inpyproject.tomlpre-commitis not needed to use the sasmodels module: it's not needed forimport sasmodels; ...to work; it shouldn't be inrequirements.txt
(I also doubt that siphash24 is a runtime dependency of sasmodels and it should also come out of that file)
There was a problem hiding this comment.
Given these comments, I have introduces a requirements-dev.txt and added pre-commit to it. I have also added the line dev = [ "build_tools/requirements-dev.txt" ] to the [tool.hatch.metadata.hooks.requirements_txt.optional-dependencies] section of the pyproject.toml, as in SasView. I will leave the decision about siphash24 to someone else.
| - uses: actions/checkout@v4 | ||
| - uses: astral-sh/ruff-action@v3 | ||
| with: | ||
| version: "latest" |
There was a problem hiding this comment.
According to https://github.com/astral-sh/ruff-action , latest is the default. This directive might not be necessary
There was a problem hiding this comment.
Without that there, we'd get an unwanted annotation on every CI run that it's using the latest version, that might not be what we want, and we should specify a version.
There was a problem hiding this comment.
As @llimeht stated, in spite of it being the default, this proved to be an issue in the SasView repo, see SasView/sasview#3605 and SasView/sasview#3608
Closes #657
The pre-commit hook provided here introduces the same functionality to SasModels as SasView/sasview#3573 does to SasView.
In addition, the recurrent linting errors in the
TwoYukawacode are resolved here. Once this PR is merged, any branch based on main from this point will not see further corrections to this code if it has not been modified.