GitHub workflows and pre-commit hooks; what should we check?

[ehogan]

In #3170, I suggested updating the GitHub workflow to perform all / some of the pre-commit checks, either by adding pre-commit run --all-files as a CI step, see Usage in continuous integration, or by using pre-commit.ci, see pre-commit.ci example.

I have been made aware of an issue in ESMValTool where the pre-commit hooks (in this case yapf) causes a conflict with codacy; codacy (specifically pydocstyle) requires 1 blank line required after class docstring (found 0) (D204), whereas yapf is the opposite.

Would it be worth:

• considering updating the GitHub workflow to perform the pre-commit checks?
• better configuring the checks so they don't cause conflicts?
• reviewing and potentially simplifying the checks to help minimise conflicts?
• making a decision on whether to use black (see Automated formatting #2161)?

I am keen to simplify / streamline this as much as possible before our scientists start converting AutoAssess metrics to ESMValTool in earnest, to minimise frustrations related to issues similar to the one described above.

Tagging @ESMValGroup/technical-lead-development-team and @valeriupredoi directly 😊

[bouweandela]

by using pre-commit.ci, see pre-commit.ci example.

In the past, our contributors have complained that they find the many different CI services confusing, so I'm not sure if we should add yet another service. The automatic formatting service offered by pre-commit.ci may not be the best choice for us because it will lead to merge conflicts between people's local branch and their branch on GitHub. Not everyone is comfortable fixing such conflicts.

I have been made aware of an issue in ESMValTool where the pre-commit hooks (in this case yapf) causes a conflict with codacy; codacy (specifically pydocstyle) requires 1 blank line required after class docstring (found 0) (D204), whereas yapf is the opposite.

This has been fixed some time ago, see google/yapf#744.

Would it be worth:

• better configuring the checks so they don't cause conflicts?
• reviewing and potentially simplifying the checks to help minimise conflicts?

You're welcome to do that and make some pull requests.

• considering updating the GitHub workflow to perform the pre-commit checks?

The main advantage of using Codacy is that the linting results are presented on a nice-looking website instead of as terminal output, which may be intimidating for some.

• making a decision on whether to use black (see Automated formatting #2161)?

Maybe you could bring that up at the upcoming workshop at SMHI ESMValGroup/Community#98 and try to find consensus there.

[bouweandela]

Is this an accurate representation of the checks happening within ESMValTool? Have I missed anything?

codespell
this is a generic spell checker, it is not specific to Python as far as I'm aware

prospector
It looks like you misinterpreted what prospector is. It is not a linter by itself, but rather a wrapper around many other Python linters, including Pylint. Maybe this misunderstanding comes from the fact that we have both tools enabled on Codacy (hence the 'Notes on Codacy Pylint' warning that you're seeing). The reason we have Pylint enabled as a separate checker as well on Codacy is that occasionally prospector will crash and return no results. This would then give a green checkmark behind a pull request with hundreds of code quality issues. By enabling Pylint, we would at least still get the Pylint results.

The reason we have two tools for Python linting is the following:

1. flake8 provides basic sanity checks that should always pass, therefore this is run along with the unit tests.
2. prospector runs more extensive code quality heuristics. Therefore it occasionally gives some false positives and that's why we're not running it along with the unit tests, but displaying them on Codacy. This gives some room for human interpretation. Codacy has the nice feature that you're only seeing issues in lines of code you changed yourself, something that may not be feasible with pre-commit because it seems to always check entire files.

A question: Would it be possible for me to install pre-commit.ci, perhaps on the https://github.com/ESMValGroup/ESMValTool-private repository, for testing purposes?

Would it be possible to use your own fork?

[bouweandela]

By the way, I'm not too keen on docformatter: generally the automatic reformatting it performs does not result in better docstrings, so if it makes your life easier, I think we could drop it.

[ehogan]

Thanks @bouweandela! 😊 Apolgies for not responding sooner; the last few weeks have been really busy for me! 😩

Using a fork is a great idea! However, I discovered that the Iris developers have already implemented this on their repositories! See https://results.pre-commit.ci/latest/github/SciTools/iris/main 😊 What do you think?

[bouweandela]

Could you please point me to examples of how

• pre-commit.ci is able to present linting output on a nice looking website instead of as terminal output
• pre-commit is able to run linting on those parts of the files that have changed only

?

[ehogan]

The Iris link I shared in my previous comment is the "nice looking website" (!) It will show the output from pre-commit formatted like you would see via a terminal but on a web page. At least that means all errors will be visible from this top level page (no more clicking through multiple links to get to the page with the error!).

If pre-commit has been configured to run linting on only the files that have changed (via the .pre-commit-config.yaml file) then it will do so via pre-commit.ci as well. In my opinion, this is one of the benefits of using pre-commit-ci; both pre-commit and pre-commit.ci are configured using the same file (so less maintenance for us!) and the behaviour of them will be the same (so no surprises for everyone!) 😁