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

Docstring improvements #245

Merged

Conversation

xaviernogueira
Copy link
Member

@xaviernogueira xaviernogueira commented Dec 6, 2023

Closes #186
@abkfenris I will be a free-agent in a week or two, and plan to contribute in this ecosystem more heavily. I figured documenting and type-hinting the codebase would be an opportunity to refresh on the inner-workings.

Changes:

  • I added Ruff's pydocstyle to our pyproject.toml. This includes docstring formatting as part of the pre-commit. I have it set to ignore the tests/ and docs/ directories as that seems fussy, but I am happy to cover those two as well.
  • I also have it set to ignore three docstring rules, D100 (requires docstrings for all modules / .py files), D107 (requires docstrings for all init.py), and D104 (simular to D107, requires all packages to have a docstring). Happy to change this. I find module docstrings a bit overkill, especially as the name of the module should be semantically relevant, and all classes / functions have docstrings.
  • I converted all docstrings to Google napolean style.
  • I added typehints where missing, I also used pyright to make check that all typehints are correct, and fixed some bad ones.
  • Very minor syntax changes to satisfy pyright best practices. No changes to logic.
  • I added pre-commit to the dev environment requirements.txt.

@xaviernogueira
Copy link
Member Author

Everything seems ready to go (all tests passing), except CodeCov.

@xaviernogueira
Copy link
Member Author

xaviernogueira commented Dec 6, 2023

Unrelated to docstring update: Since I am a bit of a perfectionist having the CodeCov failure bothered me. To address it, I looked at the coverage report and saw that the logic to catch uninitialized plugins (throws an AttributeError) was un-tested.

I decided to write a test to capture this, but in doing so, realized that the exception-handling logic was flawed!

If one passes in an uninitialized plugin, instead of making it to the try/except block, an AttributeError will be thrown as the .name attribute cannot be read (that is unless an optional plugin_name is provided to register_plugin).

I expanded the try/except block to contain both the plugin.name reading, as well as the previously captured block. I added a test for this as well.

All checks passing now 😄 @abkfenris

Copy link
Member

@abkfenris abkfenris left a comment

Choose a reason for hiding this comment

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

Thanks for chasing down all the types and cleaning up the documentation! There are a few small things, one being a place where we likely had the typing wrong before.

xpublish/rest.py Outdated Show resolved Hide resolved
tests/test_rest_api.py Outdated Show resolved Hide resolved
@xaviernogueira xaviernogueira merged commit e791d2b into xpublish-community:main Dec 6, 2023
14 checks passed
@xaviernogueira xaviernogueira deleted the docstring_improvements branch December 6, 2023 23:40
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.

Clean up Docstrings
2 participants