grass
grass copied to clipboard
OSGeo
pre-commit: Update pre-commit config and tools versions
Multiple pre-commit tools were not up-to-date.
Most annoyingly, Flake8 didn't run at all with a Python 3.12 (by default) workspace, and had a version of a couple years ago.
Since we will be changing the black yearly version soon, I made a round of changes that should be integrated before this.
I added some additional tools from the https://github.com/pre-commit/pre-commit-hooks, and enabled black formatting for jupyter notebooks.
Someone with admin rights should enable the pre-commit checks from this issue https://github.com/OSGeo/grass/issues/3255 in the short-term, as there were already some files that were out of date. (Excluding jupyter notebooks). Should I do a separate PR for these 13 files? If setting up pre-commit.ci is too complicated (meaning only one person can really administer the changes), I suggest their other way of running pre-commit, by GitHub Actions workflows so we could at least be able to do PR for improving the configuration.
Beware, pre-commit settings should be in sync with https://github.com/OSGeo/grass/blob/main/.github/workflows/python-code-quality.yml and other eventual settings.
Beware, pre-commit settings should be in sync with https://github.com/OSGeo/grass/blob/main/.github/workflows/python-code-quality.yml and other eventual settings.
I'll check tomorrow :)
https://github.com/OSGeo/grass/pull/3317 contains easily reviewable issues that can be quickly fixed, but I don't allow myself to merge my own PRs without reviews
Will probably need to have some fixes merged before this, since the GitHub Actions will enforce the quality now.
@nilason I'm a little stuck, I need an answer to continue... I had to use pipx for flake8 (I also used black that way to at the same time) since the pinned pylint had dependencies that were incompatible.
But now obviously the PR is unmergeable because of new detections. Do I apply the Python changes in another PR (that needs to be merged before) or in this one?
In general, I'd say code changes that can be updated as is, should go to separate PR(s) (if it isn't literally only one or two minor changes), which will possibly be able to be merged faster and commit history will be clearer. You could also consider split these changes to a PR for each part (flake8, black etc.). Update of black version will for example be difficult without a code format.
I'm thinking of removing the sync with the GitHub workflow since it will require a change in the required checks the way they are defined now. I'm making tests in my fork to indicate what exact changes are needed.
Here the black version is within the same year, without preview style, so no real changes happen. The original idea behind this PR, by only updating the pre-commit config, as it is usually run locally only on changed code (when it is run), is that new PRs would be in better shape in the time that we are updating the requirements of the whole repo. That's why it was not "breaking" and in steps like that.
I reverted the changes that synced the versions in the Python Code Quality workflow. I developed a solution yesterday and today, and I'm writing the PR with the changes to be made. Then another PR with the actual remaining changes.
Does this PR make sense, as it could only be highering the bar of code through pre-commit (used locally), when adding new code (except if used manually on the whole code base).
I'm sorry, can you please summarize the current state again? I don't understand why the version sync is not needed anymore. Otherwise the change looks good.
I made this PR independent of updating the CI.
This could be merged before the CI tools versions are updated, but not the other way around, since pre-commit will only apply locally on new code. If we update the CI tools versions, we need to have the current state ready (and fixed up) at the same time to not lock out any PRs.
If I finish off what I'm working on now, I'll write up the PR or PRs to update the CI in a non-breaking way, now that it is possible to do so without locking ourselves out, from a required checks point of view.
I'm starting to get used to the size of the PRs that are able to be reviewed in a timely manner before getting too complicated and that nobody feels confident enough to collaboratively decide that a PR is accepted. It seems they really need to be divided into separate PRs such as only a single reviewer with knowledge in that subject can decide. It can't be two or more reviewers with knowledge in complementary domains that can settle on approving a PR that requires both subjects. Even if sometimes it would make more sense to have a single PR since the changes are closely related. I'm thinking out loud while rereading this, this would mean that we (members/reviewers) should learn to use the "request reviewers" section in the PR, to ask them to give their opinion when we don't feel we are enough. With a requested review, the PR merging would be blocked until input from that person, or their request to review removed.
@echoix Assigning you because I don't know how to proceed, but let me know what you need from me.
Hmm... Last state of that PR was that there was some fixes to make the now working again flake8 that would be needed if it were enforced by CI. And since then, black 24.2 came out, and on the python code quality side (not in this PR anymore), there was newer Pylint to configure again from their new templates, since it changed a bit, but they can easily generate config files, even for pyproject.toml. I had a better success partially fixing the newer errors (but still being stricter than base), than to directly use our old configuration with new or a little less new versions of Pylint.