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 with ruff in pre-commit and ci linting #2586

Merged
merged 24 commits into from
Oct 26, 2023

Conversation

mraspaud
Copy link
Member

@mraspaud mraspaud commented Oct 4, 2023

This PR uses ruff instead of flake8 and mccabe.

It also add news rules for linting, so I'm expecting this to fail and start a discussion about what rules to use.

When that is done, I will fix satpy accordingly.

As first draft, here what I propose:

I'm also open to adding more checks, the full list is here:
https://docs.astral.sh/ruff/rules/

@mraspaud mraspaud self-assigned this Oct 4, 2023
@mraspaud mraspaud requested a review from djhoese as a code owner October 4, 2023 08:10
@pnuu
Copy link
Member

pnuu commented Oct 4, 2023

What triggers this change? Is it necessary to switch over for some reason, or is ruff simply better in some way?

@mraspaud
Copy link
Member Author

mraspaud commented Oct 4, 2023

What triggers this change? Is it necessary to switch over for some reason, or is ruff simply better in some way?

Ruff has much better performance. Since we run linting on every commit (when pre-commit is installed), I thought it would be nice to save computing time for everyone, and for the linting ci.

Another good thing is that is supports pyproject.toml which flake8 does not do.

See a list of benefits on the top of this page https://docs.astral.sh/ruff/

@djhoese
Copy link
Member

djhoese commented Oct 4, 2023

I am OK with adopting ruff, but I think we could drop the lint/style checks in the CI in favor of pre-commit.ci doing it all.

Also...adopt black?

@mraspaud
Copy link
Member Author

mraspaud commented Oct 4, 2023

I am OK with adopting ruff, but I think we could drop the lint/style checks in the CI in favor of pre-commit.ci doing it all.

Ok, I can do that

Also...adopt black?

Black is not a linter, this is about linting 😁

Rely on pre-commit entirely now.
@djhoese
Copy link
Member

djhoese commented Oct 4, 2023

Sure, a separate PR. But it would be run in pre-commit so it isn't way out of the realm of conversation here and is something that would automatically fix many syntax/style errors complained about here.

@mraspaud
Copy link
Member Author

mraspaud commented Oct 4, 2023

Indeed, I was just trying to avoid the question as I don't like black and I would rather not have it in satpy, which may not be a popular opinion.

I would like this PR to be about ruff, but we can make an issue about black to discuss the pros and cons of it if you like.

Copy link
Member

@djhoese djhoese left a comment

Choose a reason for hiding this comment

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

I vote for removing the linting job in CI, but otherwise I'm good with whatever rules you want to apply. Can we tell ruff what version(s) of python to target so you don't have to have the B905 rule ignored?

@gerritholl
Copy link
Collaborator

Looks good, but let's merge this after and not before the new release.

pyproject.toml Outdated Show resolved Hide resolved
Co-authored-by: Gerrit Holl <[email protected]>
@codecov
Copy link

codecov bot commented Oct 11, 2023

Codecov Report

Merging #2586 (41759a8) into main (bc32c94) will increase coverage by 0.00%.
The diff coverage is 90.64%.

@@           Coverage Diff           @@
##             main    #2586   +/-   ##
=======================================
  Coverage   95.18%   95.18%           
=======================================
  Files         354      354           
  Lines       51253    51256    +3     
=======================================
+ Hits        48786    48789    +3     
  Misses       2467     2467           
Flag Coverage Δ
behaviourtests 4.24% <1.40%> (-0.01%) ⬇️
unittests 95.81% <90.63%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
satpy/__init__.py 86.66% <100.00%> (ø)
satpy/_config.py 96.59% <100.00%> (ø)
satpy/_scene_converters.py 92.30% <ø> (ø)
satpy/composites/cloud_products.py 95.16% <100.00%> (ø)
satpy/composites/glm.py 100.00% <100.00%> (ø)
satpy/composites/spectral.py 100.00% <100.00%> (ø)
satpy/composites/viirs.py 94.76% <100.00%> (ø)
satpy/dataset/anc_vars.py 90.00% <100.00%> (ø)
satpy/dataset/data_dict.py 92.23% <100.00%> (ø)
satpy/dataset/metadata.py 100.00% <100.00%> (ø)
... and 279 more

... and 11 files with indirect coverage changes

@mraspaud
Copy link
Member Author

Looks good, but let's merge this after and not before the new release.

Will do!

@mraspaud
Copy link
Member Author

mraspaud commented Oct 26, 2023

@djhoese @gerritholl ok, I think have to stop working on this, because it's way too time consuming for my health atm. I did not add all the rules I planned to, but the ones we have currently in main are implemented, and even a couple new ones.

If you want to review this, I highly recommend you check the changes by commit, since they should be consistent with one new rule per commit. Especially the one for pytest-style, which is a mouthful.

@djhoese
Copy link
Member

djhoese commented Oct 26, 2023

I think have to stop working on this, because it's way too time consuming for my health atm.

I was honestly surprised when you said you were starting this. This is the same reason I didn't do it earlier.

@djhoese
Copy link
Member

djhoese commented Oct 26, 2023

There are a lot of test failures.

Copy link
Member

@djhoese djhoese left a comment

Choose a reason for hiding this comment

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

Thanks for putting all the work into this. There are test failures, but otherwise this looks good to me. Let's see if we can track down these failures.

@mraspaud mraspaud merged commit 652a236 into pytroll:main Oct 26, 2023
16 of 19 checks passed
@mraspaud mraspaud deleted the add-ruff branch October 26, 2023 17:44
@mraspaud mraspaud added the cleanup Code cleanup but otherwise no change in functionality label Oct 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Code cleanup but otherwise no change in functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants