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

MAINT: Changed class constructor __init__ GL08 reporting #592

Merged
merged 15 commits into from
Jan 10, 2025
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion doc/format.rst
Original file line number Diff line number Diff line change
Expand Up @@ -557,7 +557,9 @@ Class docstring
Use the same sections as outlined above (all except :ref:`Returns <returns>`
are applicable). The constructor (``__init__``) should also be documented
here, the :ref:`Parameters <params>` section of the docstring details the
constructor's parameters.
constructor's parameters. While repetition is unnecessary, a docstring for
the class constructor (``__init__``) can, optionally, be added to provide
detailed initialization documentation.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We recommended only the class docstring before. How do documentation writers decide what to put in the two docstrings?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Above my introductory pay grade, but I would stick to recommending class docstring only? The changes only allow flexibility for a constructor docstring if desired.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes:

There should be one-- and preferably only one --obvious way to do it.

And all that :)


An **Attributes** section, located below the :ref:`Parameters <params>`
section, may be used to describe non-method attributes of the class::
Expand Down
3 changes: 3 additions & 0 deletions doc/validation.rst
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,9 @@ inline comments:
def __init__(self): # numpydoc ignore=GL08
pass

Note that a properly formatted :ref:`class <classdoc>` docstring
silences ``G08`` for an ``__init__`` constructor without a docstring.

This is supported by the :ref:`CLI <validation_via_cli>`,
:ref:`pre-commit hook <pre_commit_hook>`, and
:ref:`Sphinx extension <validation_during_sphinx_build>`.
Expand Down
141 changes: 141 additions & 0 deletions numpydoc/tests/test_validate.py
Original file line number Diff line number Diff line change
Expand Up @@ -1198,6 +1198,113 @@ def missing_whitespace_after_comma(self):
"""


class ConstructorDocumentedInClassAndInit:
"""
Class to test constructor documented via class and constructor docstrings.

A case where both the class docstring and the constructor docstring are
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 ConstructorDocumentedInClassAndInit.
"""

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 ConstructorDocumentedInClassAndInit.
"""


class ConstructorDocumentedInClass:
"""
Class to test constructor documented via class docstring.

Useful to ensure that validation of `__init__` does not signal GL08,
when the class docstring properly documents the `__init__` constructor.

Parameters
----------
param1 : int
Description of param1.

See Also
--------
otherclass : A class that does something else.

Examples
--------
This is an example of how to use ConstructorDocumentedInClass.
"""

def __init__(self, param1: int) -> None:
pass


class ConstructorDocumentedInClassWithNoParameters:
"""
Class to test constructor documented via class docstring with no parameters.

Useful to ensure that validation of `__init__` does not signal GL08,
when the class docstring properly documents the `__init__` constructor.

See Also
--------
otherclass : A class that does something else.

Examples
--------
This is an example of how to use ConstructorDocumentedInClassWithNoParameters.
"""

def __init__(self) -> None:
pass


class IncompleteConstructorDocumentedInClass:
"""
Class to test an incomplete constructor docstring.

This class does not properly document parameters.
Unnecessary extended summary.

See Also
--------
otherclass : A class that does something else.

Examples
--------
This is an example of how to use IncompleteConstructorDocumentedInClass.
"""

def __init__(self, param1: int):
pass


class TestValidator:
def _import_path(self, klass=None, func=None):
"""
Expand Down Expand Up @@ -1536,6 +1643,40 @@ 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",
[
("ConstructorDocumentedInClass", tuple(), ("GL08",), tuple()),
("ConstructorDocumentedInClassAndInit", tuple(), ("GL08",), tuple()),
(
"ConstructorDocumentedInClassWithNoParameters",
tuple(),
("GL08",),
tuple(),
),
(
"IncompleteConstructorDocumentedInClass",
("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."""
Expand Down
18 changes: 17 additions & 1 deletion numpydoc/validate.py
Original file line number Diff line number Diff line change
Expand Up @@ -637,7 +637,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)
Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like Validator._load_obj(f"{doc.code_obj.__module__}.{doc.code_obj.__qualname__}") might do it, or maybe simpler Validator._load_obj(doc.name[:-9]) could work. @mattgebert do you want to try it (probably the second one)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@larsoner I don't think the second one works; doc.name[:-9] is numpydoc.tests.test_validate.__init__ when testing IncompleteConstructorDocumentedInClass.__init__ (is this expected for doc.name, not including the class name?). This means you can't resolve the class without the __qualname__ like you used in the first suggestion. Two equivalent options seem to be:

        cls = Validator._load_obj(f"{doc.code_obj.__module__}.{cls_name}")

        cls = Validator._load_obj(f"{doc.name[:-9]}.{cls_name}")

Any preference?

Copy link
Collaborator

Choose a reason for hiding this comment

The 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,
Expand Down