Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace flake8, yapf, and isort with ruff #3893

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

bouweandela
Copy link
Member

@bouweandela bouweandela commented Jan 30, 2025

Description

Replace yapf and isort by ruff format and flake8 by ruff check and runs these via a pre-commit hook. To make use of the automatic formatting, it is highly recommended that everyone runs

pre-commit install

after this is merged. This will make sure that any changes are up to standards when you are committing.

To make the pre-commit hooks run without errors and without introducing many manual changes in one pull request, I've had to disable the R formatter and linter, the NCL linter, and the YAML linter for now in the pre-commit setup. In a follow-up pull request, I can enable these again.

As previously discussed in #2161 and #3739.

Documentation: https://esmvaltool--3893.org.readthedocs.build/en/3893/community/code_documentation.html#python

Git branch upgrade instructions (once this has been merged)

If you experience many or difficult merge conflicts in your existing branches, the following procedure is recommended:

  1. Create a copy of those files you changed in your branch w.r.t. the main branch, e.g. by copying your entire clone of the repository to another directory: cp -a ESMValTool ESMValTool_backup
  2. Create a new branch off the main branch
  3. Copy the copies you created in step 1 over the existing files, e.g. cp ESMValTool_backup/esmvaltool/recipes/examples/recipe_python.yml ESMValTool/esmvaltool/recipes/examples/recipe_python.yml, etc
  4. Run pre-commit install if you haven't done so yet
  5. Run pre-commit run -a to automatically format your changes
  6. Carefully review your changes to check that you are not undoing any recent changes that happened between the time you created your original branch and now
  7. Commit and continue working with your new branch
  8. If you already have an open pull request, you can either close that and open a new one with your new branch, or use some git tricks to reset your old branch to your new branch. WARNING: only run these commands after creating a backup as recommended in step one, as you will lose your old branch if you do this. The commands to do that are git checkout my_old_feature_branch, git reset --hard my_new_feature_branch (this will change your old feature branch so it is the same as the new feature branch), git push --force (this will force an update of your old feature branch on GitHub).

or see ESMValGroup/ESMValCore#2524 for an alternative if you prefer not to copy files manually and are not afraid of fixing some merge conflicts.


Before you get started

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.

@bouweandela
Copy link
Member Author

I've reviewed all the Codacy issues and they are all existing issues, no new issues have been introduced by the ruff formatting.

@bouweandela bouweandela marked this pull request as ready for review January 31, 2025 08:31
@bouweandela bouweandela requested review from alistairsellar, ehogan and a team as code owners January 31, 2025 08:31
@valeriupredoi
Copy link
Contributor

I've reviewed all the Codacy issues and they are all existing issues, no new issues have been introduced by the ruff formatting.

you reviewed all 303 of them?? Poor Bouwe...

Copy link
Contributor

@valeriupredoi valeriupredoi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as much as I'd like to approve straightaway, I feel like this one warrants a wee bit of a discussion, amongst us, so am putting a temp blocker, just in case.

My two pennies about this is it's gonna be hard to have a pre-commit+ruff standard most of the times, in the beginning, that is. Just as we did with Codacy, we'll have to chase developers to fix their now Ruff code. But we can try and see how it goes. Change is hard, but it ought to happen, in the end.

@valeriupredoi
Copy link
Contributor

In a follow-up pull request, I can enable these again.

I'd honestly not do that, it's already hard to maintain code functionality for R and NCL, now we'll have to care about style too (AFAIK that'd be a new thing we'd not had until now, right?)

@bouweandela
Copy link
Member Author

bouweandela commented Jan 31, 2025

I feel like this one warrants a wee bit of a discussion, amongst us, so am putting a temp blocker, just in case.

There has been a considerable amount of discussion already over the past few years, e.g. at workshops and at the @ESMValGroup/technical-lead-development-team meetings and so far people seem to be in favor. We've asked the online community for input see e.g. #2161 (comment). So far there are 8 votes in favour and zero against. Using modern tooling (i.e. ruff and pre-commit) will make it easier for people to contribute to our project because this is what they are used to from other projects.

In a follow-up pull request, I can enable these again.

I'd honestly not do that, it's already hard to maintain code functionality for R and NCL, now we'll have to care about style too (AFAIK that'd be a new thing we'd not had until now, right?)

No, we have unit tests that enforce style for R and NCL, but somehow these seem to have missed a few issues.

@valeriupredoi
Copy link
Contributor

ah I knew there was some approval - I remember us sitting around the fire and smoking the Ruff pipe at one of our TLTs - sorry, 🐠 memory here. Perfect! Let me approve then 🍺

@valeriupredoi
Copy link
Contributor

No, we have unit tests that enforce style for R and NCL, but somehow these seem to have missed a few issues.

That's OK - we will prob end up "on your own"-ing those packages anyway

Copy link
Contributor

@valeriupredoi valeriupredoi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to fix the Github Actions workflows too, I can do it in a separate PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants