-
-
Notifications
You must be signed in to change notification settings - Fork 162
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 validation for optional parameters #431
base: main
Are you sure you want to change the base?
Conversation
I think there should also be the inverse check to what has been implemented, i.e. if a parameter is documented as optional/default then check that the parameter has a default in the signature. However, it isn't super clear to me with the usage of the set-like notation. Set-like notation [my terminology] is described as:
I haven't done a survey yet, but I heavily suspect that users may also be using this for 'fixed set of values' but without a default. Edit: I have implemented the reverse check but not when the set-like notation is used. |
…mpydoc into validate-optional-params
@@ -922,6 +947,30 @@ def bad_parameter_spacing(self, a, b): | |||
""" | |||
pass | |||
|
|||
def no_documented_optional(self, a=5): |
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.
I think this is too strict of an interpretation of "optional".
For example, it is quite common to use mykwarg=None
to indicate some form of default behavior, in which case I think it's safe to call mykwarg
"optional". In this case however, just because a
is a keyword argument does not necessarily mean that it's optional. For example, if a=None
would raise an exception (e.g. as might be expected in the head1
function above) then I don't think it's necessarily correct to call the a
kwarg "optional".
In other words, I think there needs to be a little more discussion in determining what "optional" really means in the context of the docstring standard, and if there's a distinction between optional/non-optional kwargs. I suspect that some projects do make such a distinction in their documentation.
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.
This implementation would treat mykwarg=None
as an optional parameter too. I can add a test here to ensure this.
These meet the definition of optional in that users can opt to not provide it. There is no guarantee that this is a suitable choice when it is optional. In my opinion, if the function raises an error based on a default value, then this should be documented, probably in the Raises
section.
I welcome a discussion on this, should it be here or in a separate issue?
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.
I think it is also important that projects can turn off this check if they have specific requirements where a parameter is optional in its signature but not optional in implementation. It should be more common to have have the signature match the implementation requirement.
As evidence of usefulness of this check, I ran this branch (using 3ac46bf) against https://github.com/pyvista/pyvista repo and found well over 50 parameters that were incorrectly documented with respect to the optionality of the parameter. In a non-random sampling, they were all true positive IDs. See pyvista/pyvista#3347. |
This PR adds validation check for optional parameters. If an optional parameter exists in the signature of a function, the documentation is checked for one of three options in the 'type':
optional
default
{'option1', 'option2'}
Related to #367.