-
-
Notifications
You must be signed in to change notification settings - Fork 169
MAINT: Changed class constructor __init__ GL08 reporting #592
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
Changes from 5 commits
bd64a7d
998d72f
a43b5b9
cc5fe97
ea2c5e3
3f51f53
66818ea
b22eb3b
be2bd62
296043d
e0edc18
b7eb310
de00113
abe64b2
8fb92b5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1198,6 +1198,90 @@ def missing_whitespace_after_comma(self): | |
""" | ||
|
||
|
||
class GoodConstructorInclusion: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it correct to have parameters both in class docstr and init? Maybe it's OK 🤔 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a good point, particularly if both exist. I haven't captured this complexity. I think it's logical to allow both; particularly as the class docstr can be comprehensive (i.e. listing atrributes and properties which wouldn't be listed on init). I think where this really makes a difference is in linting for UI interpret hints - for accessible hinting you want to enforce parameter documentation. For example, Pylance (vscode) will use both the class docstr and init docstr to display documentation for references / initialisation. When a init docstr hasn't been provided, initialisation hinting defaults to the class docstr. We might be missing one thing here. The current revision checks if the missing docstr belongs to a constructor, and if it is satisfied by the class docstr. Perhaps we should likewise check class docstrs, silencing parameters mismatch warnings if parameters are satisfied by an (existing) init docstr? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can perhaps think of that as some additional follow-up logic, if you want? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (I meant separate PR, so as to not slow this one down.)
mattgebert marked this conversation as resolved.
Show resolved
Hide resolved
|
||
""" | ||
Class to test optional constructor docstring inclusion. | ||
|
||
As the class docstring can define the constructor, a check should raise GL08 if a constructor docstring is defined. | ||
mattgebert marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
Parameters | ||
---------- | ||
param1 : int | ||
Description of param1. | ||
|
||
See Also | ||
-------- | ||
otherclass : A class that does something else. | ||
|
||
Examples | ||
-------- | ||
This is an example of how to use GoodConstructorInclusion. | ||
""" | ||
|
||
def __init__(self, param1: int) -> None: | ||
""" | ||
Constructor docstring with additional information. | ||
|
||
Extended information. | ||
|
||
Parameters | ||
---------- | ||
param1 : int | ||
Description of param1 with extra details. | ||
|
||
See Also | ||
-------- | ||
otherclass : A class that does something else. | ||
|
||
Examples | ||
-------- | ||
This is an example of how to use GoodConstructorInclusion. | ||
""" | ||
|
||
|
||
class GoodConstructorExclusion: | ||
mattgebert marked this conversation as resolved.
Show resolved
Hide resolved
|
||
""" | ||
Class to test optional constructor docstring exclusion. | ||
|
||
As the class docstring can define the constructor, a check should not raise GL08 if no constructor docstring is defined. | ||
|
||
Parameters | ||
---------- | ||
param1 : int | ||
Description of param1. | ||
|
||
See Also | ||
-------- | ||
otherclass : A class that does something else. | ||
|
||
Examples | ||
-------- | ||
This is an example of how to use GoodConstructorExclusion. | ||
""" | ||
|
||
def __init__(self, param1: int) -> None: | ||
pass | ||
|
||
|
||
class BadConstructorExclusion: | ||
mattgebert marked this conversation as resolved.
Show resolved
Hide resolved
|
||
""" | ||
Class to test undocumented constructor docstring. | ||
|
||
Unnecessary extended summary. | ||
|
||
See Also | ||
-------- | ||
otherclass : A class that does something else. | ||
|
||
Examples | ||
-------- | ||
This is an example of how to use BadConstructorExclusion. | ||
""" | ||
|
||
def __init__(self, param1: int): | ||
pass | ||
|
||
|
||
class TestValidator: | ||
def _import_path(self, klass=None, func=None): | ||
""" | ||
|
@@ -1536,6 +1620,34 @@ def test_bad_docstrings(self, capsys, klass, func, msgs): | |
for msg in msgs: | ||
assert msg in " ".join(err[1] for err in result["errors"]) | ||
|
||
@pytest.mark.parametrize( | ||
"klass,exp_init_codes,exc_init_codes,exp_klass_codes", | ||
[ | ||
("GoodConstructorExclusion", tuple(), ("GL08",), tuple()), | ||
("GoodConstructorInclusion", tuple(), ("GL08",), tuple()), | ||
( | ||
"BadConstructorExclusion", | ||
("GL08",), | ||
tuple(), | ||
("PR01"), # Parameter not documented in class constructor | ||
), | ||
], | ||
) | ||
def test_constructor_docstrings( | ||
self, klass, exp_init_codes, exc_init_codes, exp_klass_codes | ||
): | ||
# First test the class docstring itself, checking expected_klass_codes match | ||
result = validate_one(self._import_path(klass=klass)) | ||
for err in result["errors"]: | ||
assert err[0] in exp_klass_codes | ||
|
||
# Then test the constructor docstring | ||
result = validate_one(self._import_path(klass=klass, func="__init__")) | ||
for code in exp_init_codes: | ||
assert code in " ".join(err[0] for err in result["errors"]) | ||
for code in exc_init_codes: | ||
assert code not in " ".join(err[0] for err in result["errors"]) | ||
|
||
|
||
def decorator(x): | ||
"""Test decorator.""" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -633,7 +633,23 @@ def validate(obj_name, validator_cls=None, **validator_kwargs): | |
|
||
errs = [] | ||
if not doc.raw_doc: | ||
if "GL08" not in ignore_validation_comments: | ||
report_GL08: bool = True | ||
# Check if the object is a class and has a docstring in the constructor | ||
if doc.name.endswith("__init__") and doc.is_function_or_method: | ||
mattgebert marked this conversation as resolved.
Show resolved
Hide resolved
|
||
cls_name = doc.code_obj.__qualname__.split(".")[0] | ||
cls = getattr(importlib.import_module(doc.code_obj.__module__), cls_name) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @larsoner I don't know enough about current internals to know if this is how we do things; no utility functions / class methods for this purpose? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @larsoner I don't think the second one works;
Any preference? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Either seems okay, hopefully they're equivalent! |
||
cls_doc = Validator(get_doc_object(cls)) | ||
|
||
# Parameter_mismatches, PR01, PR02, PR03 are checked for the class docstring. | ||
# If cls_doc has PR01, PR02, PR03 errors, i.e. invalid class docstring, | ||
# then we also report missing constructor docstring, GL08. | ||
report_GL08 = len(cls_doc.parameter_mismatches) > 0 | ||
|
||
# Check if GL08 is to be ignored: | ||
if "GL08" in ignore_validation_comments: | ||
report_GL08 = False | ||
# Add GL08 error? | ||
if report_GL08: | ||
errs.append(error("GL08")) | ||
return { | ||
"type": doc.type, | ||
|
Uh oh!
There was an error while loading. Please reload this page.