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

MAINT: use ruff instead of black, flake, isort #152

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

theroggy
Copy link
Contributor

@theroggy theroggy commented Sep 4, 2024

Many projects move to ruff for linting/formatting because:

  • you only need one tool for all python linting/formatting as it supports a huge amount of rules
  • it is fast
  • supports many automatic fixes
  • very actively developed

Maybe a good idea to transition exactextract linting and formatting to it as well?

I enabled/configured the rules based on the rules used in shapely.

Remark: I temporarily excluded some pydocstyle rules before continuing work on this as it is some more work to add some more missing docstrings... and I'd rather wait to put in that effort till there is confirmation that this is a good idea ;-).

Copy link

codecov bot commented Sep 4, 2024

Codecov Report

Attention: Patch coverage is 91.89189% with 3 lines in your changes missing coverage. Please review.

Project coverage is 91.02%. Comparing base (3149ffd) to head (d98927f).

Files with missing lines Patch % Lines
python/src/exactextract/operation.py 75.00% 2 Missing ⚠️
python/src/exactextract/writer.py 90.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #152      +/-   ##
==========================================
- Coverage   91.05%   91.02%   -0.04%     
==========================================
  Files          85       85              
  Lines        6429     6438       +9     
  Branches      628      628              
==========================================
+ Hits         5854     5860       +6     
- Misses        543      546       +3     
  Partials       32       32              

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

@theroggy theroggy marked this pull request as ready for review September 4, 2024 08:40
@dbaston
Copy link
Member

dbaston commented Sep 4, 2024

Hi @theroggy . The current setup is copied from GDAL. I can appreciate that it's not state-of-the-art, but given that this is a pretty small project, my initial thought is to stick with it unless I understand a more clear advantage to switching.

@theroggy
Copy link
Contributor Author

theroggy commented Sep 4, 2024

In general "as much linting as possible" is a good idea I think, especially with an interpreted language like Python, and ruff makes this quite easy.

The reason I thought about it was actually that I noticed when starting to use exactextract in a project (cropclassification), that it isn't marked as typed for mypy... I created this PR mainly as being maybe a good first step to "upgrade" the linting... Next, the type annotations already present could be expanded where possible/useful so py.typed could be included. This way projects using exactextract can have mypy checks on the integration.

Also note that ruff mainly tries to mimic the functionalities of the most popular linting/formatting tools like black, flake, isort... and the many "plugins" that exist for these tools. So e.g. for the formatting, it is 99.9% compatible with black (ruff faq).

I am personally fan of checks on the docstrings (using the "pydocstring" tests), to make sure each function and each parameter is (and stays) properly documented.
Even in exactextract there are quite some functions/parameters that are not documented...

For GDAL I think it would be a very good idea as well to have more linting, especially on the docstrings. The effort there to get everything in line would obviously be another story than for exactextract... but maybe exactextract can act as a pilot to get some experience with it?
Note that it is also possible to just use ruff "barebones", so it just does the same as black / basic flake8, and it is then possible to activate more and more rules via the myproject.toml file in incremental steps.

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

Successfully merging this pull request may close these issues.

2 participants