Run format checks only with pre-commit

[emileten]

In CI, we run format checks both with pdm and pre-commit. Their configuration is different (see pyproject.toml versus the pre-commit config), which creates confusion and work for the user. For example, pre-commit doesn't have isort in its config, but pdm is running isort --check: as a consequence, I had a PR failing because of isort checks from pdm although I did run pre-commit locally.

Can we only run pdm run pytest in the CI, and let pre-commit do all the formatting work ?

cc @ModeSevenIndustrialSolutions

[ModeSevenIndustrialSolutions]

@emileten Thanks for showing up here and great to see you are already rolling up your sleeves. This is the first time where I'm taking an active look at the hazard repository, and can't vouch for the current setup as I was not working with the project at that time. I think you and I should compare notes on bringing it up to specification over this ticket.

I've been working on implementing a common linting and repository setup, which is still something of a work in progress, and will be happy to incorporate feedback such as yours. I posted some links in the chat the other day, but I'll add them here.

I've created this repository to hold some content related to a standard/baseline setup for Python coding projects: https://github.com/os-climate/devops-toolkit It contains some configuration files for the linting setup, some common shell scripts, and some YAML files defining a bunch of GitHub Actions workflows I've built and are currently maintaining.

Note: the isort tool is enabled and configured in the linting setup here! https://github.com/os-climate/devops-toolkit/blob/main/.pre-commit-config.yaml

We need to be careful with the various linting tools as they can fight over configuration options and perform contradictory operations; one of the things I do is make sure stuff like that is handled so the developers don't have to work that out these things out for themselves.

My advice would be that we immediately import the "devops-toolkit" repository content in hazard, then make the necessary adjustments afterwards. Joe has already done something similar for "physrisk" and the process would be similar here.

[ModeSevenIndustrialSolutions]

...and I've updated my previous comments, as I apparently had an old copy of the repository and now I've updated I can see we have migrated to PDM and pyproject.toml.

[ModeSevenIndustrialSolutions]

-> Can we only run pdm run pytest in the CI, and let pre-commit do all the formatting work?

I agree that pre-commit could/should do all the reformatting work... In repositories that we've recently cleaned up, this should already be the case. Ideally, pre-commit should run locally on each git commit operation, and use the centralised "devops-toolkit" configuration. Any changes and improvements to our setup, I'd like to apply to all OS-Climate python projects and merge into the "devops-toolkit". I also have a bootstrap script and workflow, that will do the initial setup/import, and then ongoing maintenance. It will raise a PR weekly that automatically incorporates any upstream setup changes, and keeps things in sync.

You might want to take a look at the ITR-examples repository and its GitHub Actions setup here: https://github.com/os-climate/ITR-examples/actions

Notice that we have a whole bunch of actions configured. I've also got matrix jobs to parse the pyproject.toml configuration files, so the Python versions are determined dynamically and are not hard-wired in the workflow YAML file.

Also, I am going to switch on "dependabot" in "hazard", as this gives us a bunch of useful features. Note that we also use a GitHub App called "pre-commit.ci" which ensures the linting tools are up-to-date. It is important we run that, as it's possible to bypass linting when running pre-commit hooks locally where users are using "--no-verify" or have done "pre-commit uninstall".. In some cases pre-commit.ci will auto-apply trivial reformatting to PRs and add a supplementary commit to PRs.

[emileten]

Thanks @ModeSevenIndustrialSolutions for all the info ! Sounds like a good idea to me to used centralized configuration.

I agree that pre-commit could/should do all the reformatting work

Yes totally. Right now, we have two github actions doing the same job with a different configuration : the pre-commit github action and the linting part of pdm run in the CI action. We should confine things to pre-commit.

I see there is a similar redundancy in physrisk as well. There is a pre-commit github action + linting in the CI action through tox. .

[ModeSevenIndustrialSolutions]

Please check the PR here and see if you have any feedback/comments: https://github.com/os-climate/hazard/pull/58

[ModeSevenIndustrialSolutions]

This is better: