-
Notifications
You must be signed in to change notification settings - Fork 6.9k
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
Unify parameters formatting in docstrings #3268
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@NicolasHug Thanks for fixing the situation in our docstrings. It certainly looks better now. :)
I left one comment with the points we discussed for visibility. Let me know your thoughts based on your findings.
.circleci/check_docstring_format.sh
Outdated
@@ -0,0 +1,8 @@ | |||
#!/bin/bash | |||
|
|||
match=`git grep -A 1 Parameters | grep "\-\-\-"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The proposed grep
will do the trick if Parameters
is used instead of Args: but it won't capture the other issues listed on the ticket #3003.
As discussed offline, it would be great to see if any of the popular python doc lints (darglint
, pydocstyle
or something else) can help us verify the format of pydocs for future PRs. As you pointed out, lints can enforce very strict rules that are not helpful to our community so perhaps it's worth using only the minimum checks related to missing/incorrect arguments on existing pydocs.
Let me know if you find any package that checks the above boxes or you think of an alternative approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I removed the CI check and I'll investigate how we can enforce 1) consistency between a function's signature and its documented parameters, and 2) the dostring style (google style vs numpy vs ...)
We can probably merge this PR as-is however, since it's a net improvement on its own
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Reviewed By: datumbox Differential Revision: D26156384 fbshipit-source-id: ca2dc3333e7c8a84c5661456b41213997d7812a4
Partially resolves #3003
This PR removes the remaining usage of
and related syntax in the docstings.
Also introduce a basic CI check to make sure these aren't introduced anymore.