Use ruff formatter and pre-commit

[bouweandela]

Description

Use ruff to format the Python code and pre-commit to manage formatting. Enables pre-commit.ci to check that code has been formatted correctly.

These new tools replace yapf, isort and flake8 (pyflakes and pycodestyle). I have dropped docformatter from the pre-commit hooks because it makes changes to docstrings that do not always look good (e.g. it will happily cut a sentence into two pieces to make the first half of the sentence fit on the first line of the docstring).

Please install the pre-commit hooks by going to the directory where you have checked out ESMValCore and running

pre-commit install

To format your code (and check for pycodestyle, pyflakes, and mypy issues) run:

pre-commit run -a

This will also automatically be done (on changed files only) whenever you commit your changes.

See ESMValGroup/ESMValTool#2161

Link to documentation: https://esmvaltool--2524.org.readthedocs.build/projects/ESMValCore/en/2524/contributing.html#code-quality

Git branch upgrade instructions (once this has been merged)

To upgrade your existing git branches and open pull requests to the new formatting once this pull request has been merged into the main branch, the following procedure is recommended to minimize merge conflicts:

Step 1: reformat changed Python files with ruff, this is needed to minimize merge conflicts in the next step

git checkout your-branch
pip install ruff
ruff format --config line-length=79 $(git diff --name-only origin/main... '*.py')
git add -u
git commit -m 'Apply ruff formatting to changed files' -n

Step 2: merge the main branch into your branch

git pull origin main --no-ff
# edit code to address merge conflicts, use `git add path/to/file.py` to mark as solved and then run:
git commit -n

Step 3: start using pre-commit to format and check your code

pre-commit install
pre-commit run -a
# edit code to fix any issues that ruff did not solve automatically
git add -u
git commit -m 'Fix linter issues'
git push

To test these instructions before this has been merged, you can replace main with ruff-format (this branch) in the commands above.

---

Before you get started

• ☝ Create an issue to discuss what you are going to do

Checklist

It is the responsibility of the author to make sure the pull request is ready to review. The icons indicate whether the item will be subject to the 🛠 Technical or 🧪 Scientific review.

• 🧪 The new functionality is relevant and scientifically sound
• 🛠 This pull request has a descriptive title and labels
• 🛠 Code is written according to the code quality guidelines
• 🧪 and 🛠 Documentation is available
• 🛠 Changes are backward compatible
• 🛠 Any changed dependencies have been added or removed correctly
• 🛠 The list of authors is up to date
• 🛠 All checks below this pull request were successful (except the code coverage threshold because some reformatted code is untested and Codacy which thinks existing issues are new because the code they occur is has been reformatted).

---

To help with the number pull requests:

• 🙏 We kindly ask you to review two other open pull requests in this repository

[valeriupredoi]

holy mackerel 🐟

[codecov]

Codecov Report

Attention: Patch coverage is 91.63498% with 176 lines in your changes missing coverage. Please review.

Project coverage is 94.83%. Comparing base (796a785) to head (9208d58).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
esmvalcore/_task.py	65.55%	31 Missing ⚠️
esmvalcore/_citation.py	70.27%	11 Missing ⚠️
esmvalcore/_main.py	82.53%	11 Missing ⚠️
esmvalcore/experimental/recipe_output.py	79.06%	9 Missing ⚠️
esmvalcore/experimental/recipe_info.py	68.00%	8 Missing ⚠️
esmvalcore/preprocessor/_derive/lwp.py	0.00%	8 Missing ⚠️
esmvalcore/_recipe/check.py	85.10%	7 Missing ⚠️
esmvalcore/preprocessor/_derive/vegfrac.py	0.00%	6 Missing ⚠️
esmvalcore/cmor/_fixes/ipslcm/ipsl_cm6.py	0.00%	4 Missing ⚠️
esmvalcore/cmor/_fixes/native6/era5.py	91.66%	4 Missing ⚠️
... and 41 more

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2524   +/-   ##
=======================================
  Coverage   94.83%   94.83%           
=======================================
  Files         251      251           
  Lines       14191    14191           
=======================================
  Hits        13458    13458           
  Misses        733      733           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[valeriupredoi]

seems it doesn't like Python 3.10 both Linux and OSX, lemme have a look at that, bud

[bouweandela]

Thanks! I think codespell needed an extra dependency to read the configuration file in toml format on Python 3.10. Added in fe3ff7b.

[valeriupredoi]

Thanks! I think codespell needed an extra dependency to read the configuration file in toml format on Python 3.10. Added in fe3ff7b.

🐈‍⬛ 😃

[valeriupredoi]

are we going to accept ruffly 92% coverage as the new project coverage?

[bouweandela]

are we going to accept ruffly 92% coverage as the new project coverage?

My idea was to assume that reformatting is a safe operation and does not introduce bugs.. writing tests for all the lines missing coverage seems a considerable amount of work.

[valeriupredoi]

are we going to accept ruffly 92% coverage as the new project coverage?

My idea was to assume that reformatting is a safe operation and does not introduce bugs.. writing tests for all the lines missing coverage seems a considerable amount of work.

oh for sure! But I am confused as to why the coverage drops vs the original codebase, since this PR only reformats code, and doesn't introduce any hefty functionality 😕

[bouweandela]

It's only the diff coverage that is partial, if you click the little arrow near the bottom of the Codecov comment where it says "Additional details and impacted files" it says that overall coverage is unchanged.

[valeriupredoi]

It's only the diff coverage that is partial, if you click the little arrow near the bottom of the Codecov comment where it says "Additional details and impacted files" it says that overall coverage is unchanged.

ah always found these confusing af, thanks, bud! Phew 😮‍💨

[valeriupredoi]

really hope you didn't have to do all these changes manually, bud 🤣 Cheers muchly! We have to start thinking how we do that in ESMValTool - gonna be messy. What we doing about pull requests after we merge this - they'll be conflicting like mad I imagine

[valeriupredoi]

last changes are ruffling no feathers 🪶

[schlunma]

I love it!! ❤️ This makes our code so much cleaner.

Two comments:

1. Does pre-commit.ci make any changes to the code? Or does it only check the code?
2. Should we consider tackling some of the Codacy issues here? Looks like most of them are very easy to fix. Just a suggestion, don't want to put extra work on you.

Thanks so much!!

[schlunma]

Is there some kind of reference that you could add as a comment here?

[bouweandela]

Done in 7b0f8cd

[bouweandela]

Does pre-commit.ci make any changes to the code? Or does it only check the code?

It is possible to configure pre-commit.ci such that it will automatically fix issues and push a commit to your branch. However, I disabled it for now because I am concerned that this may cause trouble for some of our contributors if they forget to install the pre-commit hooks and then pre-commit.ci commits to their branch, they continue editing and add some more commits, try to git push them, and find that they cannot because the branch on GitHub conflicts with their local branch. Of course, they could git push --force, but they may not be familiar enough with git to realize that.

Should we consider tackling some of the Codacy issues here? Looks like most of them are very easy to fix. Just a suggestion, don't want to put extra work on you.

I would prefer not to make any more changes here because there are so many already that it is almost impossible to review. ruff can also automatically fix many issues, so if we enable more ruff rules over time, many of these issues will probably be automatically fixed.

[valeriupredoi]

Does pre-commit.ci make any changes to the code? Or does it only check the code?

It is possible to configure pre-commit.ci such that it will automatically fix issues and push a commit to your branch. However, I disabled it for now because I am concerned that this may cause trouble for some of our contributors if they forget to install the pre-commit hooks and then pre-commit.ci commits to their branch, they continue editing and add some more commits, try to git push them, and find that they cannot because the branch on GitHub conflicts with their local branch. Of course, they could git push --force, but they may not be familiar enough with git to realize that.

Should we consider tackling some of the Codacy issues here? Looks like most of them are very easy to fix. Just a suggestion, don't want to put extra work on you.

I would prefer not to make any more changes here because there are so many already that it is almost impossible to review. ruff can also automatically fix many issues, so if we enable more ruff rules over time, many of these issues will probably be automatically fixed.

fully support these two from Bouwe. We may, however, need to turn on auto-pre-commit commits done by Circle (as Bouwe had it turned on for a bit until I barked at him to turn it off), for some of the older PRs that will need pre-commit be run on them, and the contributor may not be doing it or be weary/scared/not know how to do it (I reckon those aren't many, since most Core devs know their stuff). That, in the case of merge main (after this PR gets merged) will result in too many a conflict, that is

[schlunma]

However, I disabled it for now because I am concerned that this may cause trouble for some of our contributors if they forget to install the pre-commit hooks and then pre-commit.ci commits to their branch, they continue editing and add some more commits, try to git push them, and find that they cannot because the branch on GitHub conflicts with their local branch.

Perfect, this is what I wanted to hear!

Will look at #2529 now to fix the tests, then we can hopefully merge this soon.

[bouweandela]

I fixed the tests for now by pinning the pandas version to not 2.2.*.

[valeriupredoi]

I fixed the tests for now by pinning the pandas version to not 2.2.*.

Peasant solution, more peasant than my peasant rounding in #2529 🐑