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
92 changes: 90 additions & 2 deletions numpydoc/tests/test_validate.py
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ def head1(self, n=5):

Parameters
----------
n : int
n : int, default 5
Number of values to return.

Returns
Expand Down Expand Up @@ -224,7 +224,7 @@ def summary_starts_with_number(self, n=5):

Parameters
----------
n : int
n : int, default 5
4 Number of values to return.

Returns
Expand Down Expand Up @@ -506,6 +506,33 @@ def parameters_with_trailing_underscores(self, str_):
"""
pass

def optional_params(self, *, not_optional, a=None, b=2, c="Thing 1"):
"""
Test different ways of testing optional parameters.

There are three ways to document optional paramters.

Parameters
----------
not_optional : str
A keyword arg that is not optional.
a : int, optional
Default is implicitly determined.
b : int, default 5
Default is explicitly documented.
c : {"Thing 1", "Thing 2"}
Which thing.

See Also
--------
related : Something related.

Examples
--------
>>> result = 1 + 1
"""
pass

def parameter_with_wrong_types_as_substrings(self, a, b, c, d, e, f):
r"""
Ensure PR06 doesn't fail when non-preferable types are substrings.
Expand Down Expand Up @@ -955,6 +982,53 @@ 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.

"""
Missing optional in docstring.

Parameters
----------
a : int
Missing optional.
"""
pass

def documented_optional_but_kwarg(self, *, a):
"""
Missing optional in docstring.

Parameters
----------
a : int, optional
Keyword arg mislabelled as optional when there is no
default.
"""
pass

def no_documented_optional_when_None(self, a=None):
"""
Missing optional in docstring when default is None.

Parameters
----------
a : int
Missing optional.
"""
pass

def no_default_when_documented_optional(self, a, b):
"""
Missing default in signature.

Parameters
----------
a : int, optional
One way to denote optional.
b : int, default 5
Another way to denote optional.
"""
pass


class BadReturns:
def return_not_documented(self):
Expand Down Expand Up @@ -1178,6 +1252,7 @@ def test_good_class(self, capsys):
"warnings",
"valid_options_in_parameter_description_sets",
"parameters_with_trailing_underscores",
"optional_params",
"parameter_with_wrong_types_as_substrings",
],
)
Expand Down Expand Up @@ -1391,6 +1466,19 @@ def test_bad_generic_functions(self, capsys, func):
("No error yet?",),
marks=pytest.mark.xfail,
),
(
"BadParameters",
"no_documented_optional",
('Parameter "a" is optional but not documented, or vice versa',),
),
(
"BadParameters",
"no_default_when_documented_optional",
(
'Parameter "a" is optional but not documented, or vice versa',
'Parameter "b" is optional but not documented, or vice versa',
),
),
# Returns tests
("BadReturns", "return_not_documented", ("No Returns section found",)),
("BadReturns", "yield_not_documented", ("No Yields section found",)),
Expand Down
79 changes: 57 additions & 22 deletions numpydoc/validate.py
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@
"PR09": 'Parameter "{param_name}" description should finish with "."',
"PR10": 'Parameter "{param_name}" requires a space before the colon '
"separating the parameter name and type",
"PR11": 'Parameter "{param_name}" is optional but not documented, ' "or vice versa",
"RT01": "No Returns section found",
"RT02": "The first line of the Returns section should contain only the "
"type, unless multiple values are being returned",
Expand Down Expand Up @@ -294,42 +295,58 @@ def doc_all_parameters(self):

@property
def signature_parameters(self):
def add_stars(param_name, info):
"""
Add stars to *args and **kwargs parameters
"""
if info.kind == inspect.Parameter.VAR_POSITIONAL:
return f"*{param_name}"
elif info.kind == inspect.Parameter.VAR_KEYWORD:
return f"**{param_name}"
else:
return param_name

if inspect.isclass(self.obj):
if hasattr(self.obj, "_accessors") and (
self.name.split(".")[-1] in self.obj._accessors
):
# accessor classes have a signature but don't want to show this
return tuple()
return dict()
try:
sig = inspect.signature(self.obj)
except (TypeError, ValueError):
# Some objects, mainly in C extensions do not support introspection
# of the signature
return tuple()
return dict()
params = dict(sig.parameters)

if params:
first_param = next(iter(params.keys()))
if first_param in ("self", "cls"):
del params[first_param]

params = tuple(
add_stars(parameter, sig.parameters[parameter])
for parameter in sig.parameters
)
if params and params[0] in ("self", "cls"):
return params[1:]
return params

@staticmethod
def _add_stars(param_name, info):
"""
Add stars to *args and **kwargs parameters
"""
if info.kind == inspect.Parameter.VAR_POSITIONAL:
return f"*{param_name}"
elif info.kind == inspect.Parameter.VAR_KEYWORD:
return f"**{param_name}"
else:
return param_name

@property
def signature_parameters_names(self):
return tuple(
self._add_stars(param, info)
for param, info in self.signature_parameters.items()
)

@property
def optional_signature_parameters_names(self):
return tuple(
self._add_stars(param, info)
for param, info in self.signature_parameters.items()
if info.default is not inspect._empty
)

@property
def parameter_mismatches(self):
errs = []
signature_params = self.signature_parameters
signature_params = self.signature_parameters_names
all_params = tuple(param.replace("\\", "") for param in self.doc_all_parameters)
missing = set(signature_params) - set(all_params)
if missing:
Expand Down Expand Up @@ -566,7 +583,8 @@ def validate(obj_name):

for param, kind_desc in doc.doc_all_parameters.items():
if not param.startswith("*"): # Check can ignore var / kwargs
if not doc.parameter_type(param):
param_type = doc.parameter_type(param)
if not param_type:
if ":" in param:
errs.append(error("PR10", param_name=param.split(":")[0]))
else:
Expand All @@ -576,13 +594,19 @@ def validate(obj_name):
errs.append(error("PR05", param_name=param))
# skip common_type_error checks when the param type is a set of
# options
if "{" in doc.parameter_type(param):
if "{" in param_type:
continue
common_type_errors = [
("integer", "int"),
("boolean", "bool"),
("string", "str"),
]

# check that documented optional param has default in sig
if "optional" in param_type or "default" in param_type:
if param not in doc.optional_signature_parameters_names:
errs.append(error("PR11", param_name=param))

for wrong_type, right_type in common_type_errors:
if wrong_type in set(re.split(r"\W", doc.parameter_type(param))):
errs.append(
Expand All @@ -593,8 +617,19 @@ def validate(obj_name):
wrong_type=wrong_type,
)
)

errs.extend(_check_desc(kind_desc[1], "PR07", "PR08", "PR09", param_name=param))

# check param with default in sig is documented as optional
for param in doc.optional_signature_parameters_names:
param_type = doc.parameter_type(param)
if (
"optional" not in param_type
and "{" not in param_type
and "default" not in param_type
):
errs.append(error("PR11", param_name=param))

if doc.is_function_or_method:
if not doc.returns:
if doc.method_returns_something:
Expand Down