Use ruff formatter and pre-commit

bouweandela commented 2 months ago

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 https://github.com/ESMValGroup/ESMValTool/discussions/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

[x] ☝ 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.

[x] 🧪 The new functionality is relevant and scientifically sound
[x] 🛠 This pull request has a descriptive title and labels
[x] 🛠 Code is written according to the code quality guidelines
[x] 🧪 and 🛠 Documentation is available
[x] 🛠 Changes are backward compatible
[x] 🛠 Any changed dependencies have been added or removed correctly
[x] 🛠 The list of authors is up to date
[x] 🛠 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 commented 2 months ago

holy mackerel :fish:

codecov[bot] commented 2 months ago

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 :warning:
esmvalcore/_citation.py	70.27%	11 Missing :warning:
esmvalcore/_main.py	82.53%	11 Missing :warning:
esmvalcore/experimental/recipe_output.py	79.06%	9 Missing :warning:
esmvalcore/experimental/recipe_info.py	68.00%	8 Missing :warning:
esmvalcore/preprocessor/_derive/lwp.py	0.00%	8 Missing :warning:
esmvalcore/_recipe/check.py	85.10%	7 Missing :warning:
esmvalcore/preprocessor/_derive/vegfrac.py	0.00%	6 Missing :warning:
esmvalcore/cmor/_fixes/ipslcm/ipsl_cm6.py	0.00%	4 Missing :warning:
esmvalcore/cmor/_fixes/native6/era5.py	91.66%	4 Missing :warning:
... and 41 more

Additional details and impacted files

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

valeriupredoi commented 2 months ago

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

bouweandela commented 2 months ago

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

valeriupredoi commented 2 months ago

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

:black_cat: :smiley:

valeriupredoi commented 2 months ago

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

bouweandela commented 2 months ago

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 commented 2 months ago

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 :confused:

bouweandela commented 2 months ago

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 commented 2 months ago

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 :face_exhaling:

bouweandela commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 https://github.com/ESMValGroup/ESMValCore/pull/2529 now to fix the tests, then we can hopefully merge this soon.

bouweandela commented 2 months ago

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

valeriupredoi commented 2 months ago

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 🐑

ESMValGroup / ESMValCore