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

Add pydoc with ruff check that public classes should have documentation. #1034

Merged
merged 10 commits into from
Aug 30, 2024

Conversation

h-mayorquin
Copy link
Collaborator

@h-mayorquin h-mayorquin commented Aug 27, 2024

Closes #1028 (as an alternative).
For discussion:
@pauladkisson @CodyCBakerPhD

My arguments for:

  1. We already have a ruff pre-commit.
  2. https://github.com/pycqa/pydocstyle is deprecated. They advice using rust.
  3. I think is better to have less pre-commit clauses and less tools.

I personally like how fast ruff is is to check this and we can activate rules one by one. For example, we can exclude the rule D100 (forcing modules to have docstrings) that @bendichter disliked or anything else from this list:
https://docs.astral.sh/ruff/rules/#pydocstyle-d

pyproject.toml Outdated Show resolved Hide resolved
@CodyCBakerPhD
Copy link
Member

Didn't know ruff supported that

Can you check if it respects inheritance (docstring in parent but not in child)? That was one of the reasons we made the separate tool - at the time as I recall other linters required the docstrings regardless

@CodyCBakerPhD
Copy link
Member

Also does it apply to public methods?

@h-mayorquin
Copy link
Collaborator Author

Didn't know ruff supported that

Can you check if it respects inheritance (docstring in parent but not in child)? That was one of the reasons we made the separate tool - at the time as I recall other linters required the docstrings regardless

It does not. Check the hdmf changes in this PR.

But I think that's a good thing. The corresponding # noqa: (e.g. # noqa: D101) should be added in those cases. I think this works as a check that the developer knows what they are doing and that the docstring does not require changes specific to the context of the function/class (as per the discussion on the meeting today about docstring consistency)

@h-mayorquin
Copy link
Collaborator Author

h-mayorquin commented Aug 27, 2024

Also does it apply to public methods?

@CodyCBakerPhD
That's another rule.
https://docs.astral.sh/ruff/rules/#pydocstyle-d

Which is nice because we can move forward piece-wise.

@pauladkisson
Copy link
Member

And this would be an alternative to #1028 right?

@h-mayorquin
Copy link
Collaborator Author

And this would be an alternative to #1028 right?

Right.

@h-mayorquin h-mayorquin marked this pull request as ready for review August 27, 2024 21:21
pyproject.toml Outdated Show resolved Hide resolved
Copy link
Member

@pauladkisson pauladkisson left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@h-mayorquin h-mayorquin enabled auto-merge (squash) August 30, 2024 15:17
@h-mayorquin h-mayorquin merged commit 39c1c2a into main Aug 30, 2024
34 of 35 checks passed
@h-mayorquin h-mayorquin deleted the add_ruff_doc_for_classess branch August 30, 2024 18:43
Copy link

codecov bot commented Nov 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.42%. Comparing base (d1e3815) to head (3a6ddb8).
Report is 71 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1034   +/-   ##
=======================================
  Coverage   90.42%   90.42%           
=======================================
  Files         129      129           
  Lines        7924     7924           
=======================================
  Hits         7165     7165           
  Misses        759      759           
Flag Coverage Δ
unittests 90.42% <100.00%> (ø)

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

Files with missing lines Coverage Δ
...datainterfaces/ecephys/axona/axonadatainterface.py 89.02% <ø> (ø)
.../ecephys/cellexplorer/cellexplorerdatainterface.py 85.38% <ø> (ø)
...rfaces/ecephys/neuralynx/neuralynxdatainterface.py 85.33% <ø> (ø)
...terfaces/ecephys/spikeglx/spikeglxdatainterface.py 94.23% <ø> (ø)
...interfaces/ophys/brukertiff/brukertiffconverter.py 100.00% <ø> (ø)
src/neuroconv/tools/hdmf.py 96.62% <100.00%> (ø)
src/neuroconv/tools/path_expansion.py 96.92% <100.00%> (ø)
...c/neuroconv/tools/testing/data_interface_mixins.py 95.33% <ø> (ø)
src/neuroconv/tools/testing/mock_interfaces.py 100.00% <ø> (ø)
src/neuroconv/utils/json_schema.py 96.87% <ø> (ø)

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.

3 participants