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 darglint with pydoclint #124

Merged
merged 5 commits into from
Aug 29, 2023
Merged

Conversation

llucax
Copy link
Contributor

@llucax llucax commented Aug 25, 2023

The darglint project is not maintained anymore, and even when there is a fork that is trying to keep it alive, it is still extremely slow.

The new project pydoclint was created recently but advancing rapidly, and already in better shape than darglint and way faster.

To be able to add ignore comments in the code, pydoclint needs to be run via flake8. Because we need to run flake8 already, and pydocstyle also can run via flake8, we are merging both in one flake8 call, so we only need to read the files once.

Performance running on the for the SDK src directory only:

darling 20s (1x)
pydoclint 0.2s (100x)
flake8 (basic checks + pydocstyle + pydoclint) 0.6s (33x)

As a side effect, we are also enabling other base flake8 checks. There is probably some overlap with pylint, but flake8 is very fast anyway, so it shouldn't be noticed (for the SDK, flake8 basic checks + pydocstyle + pydoclint runs in 0.6s and pylint alone runs in 15s). At some point we might want to disable the duplicated checks in pylint to see if it speeds up pylint.

This commit also renames the dev-docstrings optional dependency to dev-flake8 because now is not checking docstrings exclusively.

@llucax llucax requested a review from a team as a code owner August 25, 2023 21:32
@llucax llucax requested a review from Marenz August 25, 2023 21:32
@llucax llucax self-assigned this Aug 25, 2023
@llucax llucax added this to the v0.6.0 milestone Aug 25, 2023
@github-actions github-actions bot added the part:tests Affects the unit, integration and performance (benchmarks) tests label Aug 25, 2023
@llucax llucax requested a review from shsms August 25, 2023 21:32
@llucax

This comment was marked as outdated.

@llucax llucax changed the title Replace darglint with pydocstyle Replace darglint with pydoclint Aug 28, 2023
@llucax

This comment was marked as outdated.

@llucax llucax added part:docs Affects the documentation part:tooling Affects the development tooling (CI, deployment, dependency management, etc.) type:enhancement New feature or enhancement visitble to users part:nox Affects the configuration of nox part:cookiecutter Affects the generation of projects using cookiecutter labels Aug 28, 2023
@Marenz

This comment was marked as outdated.

@github-actions github-actions bot removed part:docs Affects the documentation part:tooling Affects the development tooling (CI, deployment, dependency management, etc.) part:nox Affects the configuration of nox part:cookiecutter Affects the generation of projects using cookiecutter labels Aug 28, 2023
@llucax

This comment was marked as outdated.

@llucax

This comment was marked as outdated.

@llucax
Copy link
Contributor Author

llucax commented Aug 28, 2023

Bug in pydoclint fixed! 🎉

This is now passing tests!

@llucax
Copy link
Contributor Author

llucax commented Aug 28, 2023

Enabled auto-merge.

@llucax llucax enabled auto-merge August 28, 2023 15:07
The `darglint` project is not maintained anymore, and even when there is
a fork that is trying to keep it alive, it is still extremely slow.

The new project `pydoclint` was created recently but advancing rapidly,
and already in better shape than `darglint` and way faster (0.2s vs 20s
for the SDK, 100x improvement).

To be able to add ignore comments in the code, `pydoclint` needs to be
run via `flake8`. Because we need to run `flake8` already, and
`pydocstyle` also can run via `flake8`, we are merging both in one
`flake8` call, so we only need to read the files once.

As a side effect, we are also enabling other base `flake8` checks. There
is probably some overlap with `pylint`, but `flake8` is very fast
anyway, so it shouldn't be noticed (for the SDK, `flake8` basic checks
+ `pydocstyle` + `pydoclint` runs in 0.6s and `pylint` alone runs in
15s). At some point we might want to disable the duplicated checks in
`pylint` to see if it speeds up `pylint`.

This commit also renames the `dev-docstrings` optional dependency to
`dev-flake8` because now is not checking docstrings exclusively.

Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
`pydocstyle` can now use the built-in `tomllib`, and now the
configuration is read indirectly by `flake8` anyway.

Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
Some are just `flake8` base checks, and some are found by `pydoclint`
(issues that `darglint` didn't catch):

* Line too long.
* Ununsed imports.
* Ambiguous variable name (`l` could be read as L or the number one with
  some fonts).
* Missing argument documentation.

Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
It requires some extra steps when updating.

Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
@@ -16,6 +16,8 @@

### Cookiecutter template

- CI: The `nox` job now uses a matrix to run the different `nox` sessions in parallel. If you use branch projection with the `nox` job you need to update the rules to include each matrix job.
Copy link
Contributor

Choose a reason for hiding this comment

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

oh nice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

2x speedup for this repo CI.

@llucax llucax added this pull request to the merge queue Aug 29, 2023
Merged via the queue into frequenz-floss:v0.x.x with commit 7a6802f Aug 29, 2023
10 checks passed
@llucax llucax deleted the pydocstyle branch August 29, 2023 09:06
@llucax llucax linked an issue Aug 29, 2023 that may be closed by this pull request
@llucax llucax removed this from the v0.6.0 milestone Aug 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
part:tests Affects the unit, integration and performance (benchmarks) tests type:enhancement New feature or enhancement visitble to users
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace darglint with ruff or pydoclint
2 participants