Skip to content

Conversation

@strickvl
Copy link
Contributor

@strickvl strickvl commented Mar 25, 2022

I added a pre-commit check that ensures all methods contain correct docstrings as per our formatting and that arguments in the method/function signatures match what are specified in the docstring.

I implemented several modules to start with as you can see in the files, but merging develop added in a bunch more failing code so you'll see that the darglint / pydocstyle checks are only running for two modules. I will add smaller scoped PRs over the coming weeks to cover the rest.

The docstring checks now run as part of the lint script, but I added a separate docstring.sh script for the purpose of working on the specific task of fixing docstrings (so that you don't have to also run things like mypy that take extra time).

I'll also circulate a Notion document so that the implications / decisions around this will be clear.

Pre-requisites

Please ensure you have done the following:

  • I have read the CONTRIBUTING.md document.
  • If my change requires a change to docs, I have updated the documentation accordingly.
  • If I have added an integration, I have updated the integrations table.
  • I have added tests to cover my changes.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Other (add details above)

@github-actions github-actions bot added the internal To filter out internal PRs and issues label Mar 25, 2022
@strickvl strickvl added the documentation Improvements or additions to documentation label Mar 26, 2022
Copy link
Contributor

@htahir1 htahir1 left a comment

Choose a reason for hiding this comment

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

Cool. But if we are using this tool, we need a way to use it outside of just the pre-commit. We need it in the GitHub actions via a scripts/docstrings-check.sh or something. We can then also use that script locally without pre-commit. That means we need to add it to the pyproject.toml. So its a bit more work :-)

@strickvl
Copy link
Contributor Author

@htahir1 Yes I'll add it to the script as well. This was very much a draft PR for the moment. Our codebase has hundreds of methods that need fixing, so I'll work on this slowly on the side as it's not such a high-priority thing needing an immediate fix.

@strickvl strickvl marked this pull request as ready for review May 30, 2022 10:38
@strickvl strickvl requested review from htahir1 and schustmi May 30, 2022 10:38
@strickvl strickvl requested review from AlexejPenner and htahir1 and removed request for htahir1 May 30, 2022 10:53
@strickvl strickvl changed the title Add docstring checks to pre-commit script Add docstring checks to pre-commit script May 30, 2022
@strickvl strickvl merged commit 3d5a9ce into develop May 30, 2022
@strickvl strickvl deleted the misc/docstring-checks branch May 30, 2022 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation internal To filter out internal PRs and issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants