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

Enforce google docstrings #2294

Merged
merged 52 commits into from
Sep 17, 2024
Merged

Enforce google docstrings #2294

merged 52 commits into from
Sep 17, 2024

Conversation

quaquel
Copy link
Member

@quaquel quaquel commented Sep 13, 2024

This PR enforces Google docstrings in ruff, enables parsing of Google docstrings in building the documentation for readthedocs, and (the real work) fixes all documentation to be in line with the Google docstring. While going through all the docstrings, I have made various minor modifications to improve wording.

I have used noqa D103 in quite a few places (e.g., outdated parts of the code, various tests, solara parts that I don't fully know). This means that some parts don't have a docstring yet.

@quaquel quaquel added the docs Release notes label label Sep 13, 2024
@Corvince
Copy link
Contributor

Awesome effort! That is something that has bugged me for some time and finally some effort to fix this!

I think instead of mixing and matching our own way of writing docstrings we should strive to fully adapt a style guide and I think the numpy style guide would be most appropriate and I see this most used in the scientific community.

Note that with a clear structure of the docstring format the visual representation on for example readthedocs can become a separate thought. Take for example psygnal: It uses numpy docstring format at least for the parameters (link), but renders completely different on readthedocs (with bulleted lists, links to type definitions, etc.)

@quaquel
Copy link
Member Author

quaquel commented Sep 13, 2024

I think instead of mixing and matching our own way of writing docstrings we should strive to fully adapt a style guide and I think the numpy style guide would be most appropriate and I see this most used in the scientific community.

Judging the current docstrings, it seems closests to the google docstring standard. So I sort of implicitly assumed that as the standard while working on MESA. I personally also prefer numpy. It would take a few hours to rewrite everything to numpy if all maintainers agree to use that.

Copy link
Member

@EwoutH EwoutH left a comment

Choose a reason for hiding this comment

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

Awesome, always love documentation improvements!

Also agree on the style guide. Especially to get things as bulleted lists correct consistently.

@Corvince
Copy link
Contributor

It would take a few hours to rewrite everything to numpy if all maintainers agree to use that.

There are actually several docstring converters you can find on pypi. Not sure how good they work

@EwoutH
Copy link
Member

EwoutH commented Sep 13, 2024

I would support using the NumPy Style guide for docstring formatting and numpydoc for Sphinx / Readthedocs rendering. Especially if we can enforce it in CI (preferably pre-commit).

@Corvince
Copy link
Contributor

I'll see what I can do about the solara stuff, most docstrings there are also way outdated since my refactoring!

Since this PR here is basically unreviewable due to its size, I guess I will do that in a separate PR. So for the time being you just add some noqa D to solara, is this your understanding as well?

@quaquel
Copy link
Member Author

quaquel commented Sep 17, 2024

Since this PR here is basically unreviewable due to its size, I guess I will do that in a separate PR. So for the time being you just add some noqa D to solara, is this your understanding as well?

Yes that makes the most sense and is what I have done now.

@quaquel quaquel changed the title remove methods from class docstrings and make attributes bulleted lists Enforce google docstrings Sep 17, 2024
@quaquel
Copy link
Member Author

quaquel commented Sep 17, 2024

If there are no objections, I plan to merge this PR at the close of business today. All tests and ruff stuff passes. I have checked readthedocs also and this looks much cleaner now.

Copy link
Member

@EwoutH EwoutH left a comment

Choose a reason for hiding this comment

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

Skimmed through it, looks great in general!

docs/conf.py Outdated Show resolved Hide resolved
@rht
Copy link
Contributor

rht commented Sep 17, 2024

I have a class of comments related to the non-specific noqa comments. Shouldn't they be noqa: D103? But this is not blocking the merge of this PR, and can be done in a subsequent PR, since this PR is already too big.

@quaquel
Copy link
Member Author

quaquel commented Sep 17, 2024

I have a class of comments related to the non-specific noqa comments. Shouldn't they be noqa: D103? But this is not blocking the merge of this PR, and can be done in a subsequent PR, since this PR is already too big.

I'll make a pass to make the noqa's specific. Will also be easier in the future to find them.

docs/conf.py Outdated
@@ -1,3 +1,4 @@
# noqa D103
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the ':' is necessary, # noqa: D103, but haven't tested.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for pointing this out. I have fixed it everywhere. This noqa stuff is quite new to me.

docs/conf.py Outdated Show resolved Hide resolved
@quaquel quaquel merged commit 3fce592 into projectmesa:main Sep 17, 2024
11 of 12 checks passed
@Corvince
Copy link
Contributor

Over 1000 changed lines! Awesome work @quaquel , thanks for doing this

@quaquel quaquel mentioned this pull request Sep 18, 2024
@EwoutH EwoutH added the ci Release notes label label Sep 20, 2024
@quaquel quaquel deleted the docstrings branch October 24, 2024 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci Release notes label docs Release notes label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants