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

Introduce docformatter and ruff #405

Merged
merged 7 commits into from
Sep 16, 2024
Merged

Introduce docformatter and ruff #405

merged 7 commits into from
Sep 16, 2024

Conversation

felixhekhorn
Copy link
Contributor

I apologize for this gigantic PR, but I believe we want this and hopefully nothing should have happened.

  1. By stalking @alecandido I found out about docformatter. We had so far only a soft rule that docstrings should follow the numpy convention, with this PR it is becoming a strict requirement and docformatter will do the job. Thus ~95% of the changes are a) removing new lines in docstrings or b) adding a . here and there. I did not go through the whole documentation to see, if everything went well (and for sure there are some hickups here and there), but I trust in the docformatter developers.
  2. while doing that I also found out that pydocstyle got deprecated (and which I thought would do the job) in favor of ruff, which is then introduced along side in this PR. This has triggered another few changes as the new linter complained about new things and which I had to adjust by hand.

Not sure on how much of a review is required (given the amount of files), but maybe tell me if you agree with the idea.

@felixhekhorn felixhekhorn added documentation Improvements or additions to documentation benchmarks Benchmark (or infrastructure) related labels Sep 13, 2024
@alecandido
Copy link
Member

Ruff is also doing pydocstyle job, but it has been developed as a Pylint replacement. You could really consider getting rid of Pylint, and adopt a unique tool.

In principle, the workflows are Pylint-agnostic, as they rely on poe lint (so, you should just replace the script). There is only an irrelevant name of a step mentioning explicitly Pylint
https://github.com/search?q=repo%3ANNPDF%2Fworkflows%20pylint&type=code
(in case, you can always upgrade even that, but it's also not required)

@alecandido
Copy link
Member

Ruff can be used to replace Flake8 (plus dozens of plugins), Black, isort, pydocstyle, pyupgrade, autoflake, and more, all while executing tens or hundreds of times faster than any individual tool.

https://docs.astral.sh/ruff/
https://docs.astral.sh/ruff/integrations/#pre-commit

@felixhekhorn
Copy link
Contributor Author

Ruff can be used to replace Flake8 (plus dozens of plugins), Black, isort, pydocstyle, pyupgrade, autoflake, and more, all while executing tens or hundreds of times faster than any individual tool.

https://docs.astral.sh/ruff/ https://docs.astral.sh/ruff/integrations/#pre-commit

While pydocstyle and pyupgrade were already dropped, dfe7c79 additionally dropps isort and black (I will need to get used to the fact that black will no longer be around after such a long time 🙃 ) - note that there a few changes in the format, but as long as someone is doing the job I don't care about the actual format

@felixhekhorn
Copy link
Contributor Author

Note that 8bcee93 revealed a naming problem for which I don't have better solution then the one proposed ... other ideas are welcome

Copy link
Collaborator

@giacomomagni giacomomagni left a comment

Choose a reason for hiding this comment

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

I had a quick look and indeed the changes are mostly only in docs, with some few variables l renamed (?)...
I see you are still keeping blacken-docs.

@felixhekhorn
Copy link
Contributor Author

felixhekhorn commented Sep 16, 2024

I see you are still keeping blacken-docs.

1149f18 drops blacken-docs in favor of the ruff equivalent.

I had a quick look and indeed the changes are mostly only in docs, with some few variables l renamed (?)...

yes, the new linter complained (also pylint would issue a warning) as it considers l an ambiguous variable name (i.e. can be confused with I, 1,| in some fonts); however having a slightly more verbose variable name was an easy fix and even a reasonable one

Copy link
Collaborator

@giacomomagni giacomomagni left a comment

Choose a reason for hiding this comment

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

Okay LGTM.

@felixhekhorn felixhekhorn merged commit 9a4504a into master Sep 16, 2024
8 checks passed
@felixhekhorn felixhekhorn deleted the use-docformatter-ruff branch September 16, 2024 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
benchmarks Benchmark (or infrastructure) related documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants