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

Add validation for optional parameters #431

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

MatthewFlamm
Copy link
Contributor

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':

  1. Use of optional
  2. Use of default
  3. Use of {'option1', 'option2'}

Related to #367.

@MatthewFlamm
Copy link
Contributor Author

MatthewFlamm commented Sep 20, 2022

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:

When a parameter can only assume one of a fixed set of values, those values can be listed in braces, with the default appearing first:

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.

@@ -922,6 +947,30 @@ def bad_parameter_spacing(self, a, b):
"""
pass

def no_documented_optional(self, a=5):
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 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.

Copy link
Contributor Author

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?

Copy link
Contributor Author

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.

@MatthewFlamm
Copy link
Contributor Author

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.

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.

3 participants