MAINT: Changed class constructor init GL08 reporting #592

mattgebert · 2024-12-11T13:06:29Z

Changed validate function in validate.py for empty docstring error GL08 ("The object does not have a docstring"), specifically for __init__ constructors. If an empty docstring for __init__, will first check if class docstring is satisfied for parameter mismatches, before deciding to report GL08 (if mismatches exist, i.e. incomplete documentation).

Implemented new docstring test classes in test_validate.py to verify correct functionality.
Also updated readthedocs format.rst and validation.rst to reflect new changes to constructor checking behavior.

Solves #591.

Note, I think a modification to the readthedocs https://numpydoc.readthedocs.io/en/latest/validation.html#ignoring-validation-checks-with-inline-comments ought to be made

P.s. First time pull requesting to a public repo, let me know if anything is insufficient.

…lass docstring isn't complete

larsoner · 2024-12-16T16:27:34Z

Had a quick look at the code and this looks reasonable to me!

Note, I think a modification to the readthedocs https://numpydoc.readthedocs.io/en/latest/validation.html#ignoring-validation-checks-with-inline-comments ought to be made

Sounds good, do you want to add it here?

mattgebert · 2024-12-17T03:19:58Z

@larsoner I've added new doc comments in the diff in format.rst and validation.rst. I'll repeat the modifications here:

format.rst -> `Documenting classes`

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. The class docstring does not need to be repeated  <----
in a constructor, but is optional. <-----

validation.rst -> `Ignoring Validation Checks with Inline Comments`

def __init__(self):  # numpydoc ignore=GL08
    pass

Note, if the :ref:`class <classdoc>` docstring properly documents the <----
constructor, the ``GL08`` will be ignored by default. <----

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>`.

larsoner · 2024-12-17T16:27:34Z

@larsoner I've added new doc comments in the diff

I think you maybe forgot to push (?), I don't see the changes in the files changed tab

larsoner · 2024-12-17T16:28:21Z

Ohh wait nevermind you already made the changes before I just didn't notice. And failed to notice again! 🤦

larsoner

@stefanv did you want to look?

stefanv

The general gist of this is correct, I think. I left some comments that I think may help future developers (ourselves, likely 😅) know what we did here.

doc/format.rst

doc/validation.rst

numpydoc/tests/test_validate.py

stefanv · 2024-12-17T19:59:47Z

numpydoc/tests/test_validate.py

@@ -1198,6 +1198,90 @@ def missing_whitespace_after_comma(self):
        """


+class GoodConstructorInclusion:


Is it correct to have parameters both in class docstr and init? Maybe it's OK 🤔

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?

We can perhaps think of that as some additional follow-up logic, if you want?

(I meant separate PR, so as to not slow this one down.)

numpydoc/tests/test_validate.py

stefanv · 2024-12-17T20:02:42Z

numpydoc/validate.py

+        # 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:
+            cls_name = doc.code_obj.__qualname__.split(".")[0]
+            cls = getattr(importlib.import_module(doc.code_obj.__module__), cls_name)


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

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

…e conditions they're used to check

…uctor checking

…hen the constructor has no parameters, also modified docstring descriptions for clarity

…endency

stefanv

@larsoner Would appreciate your eyes on lines 641 through 652. Otherwise LGTM.

doc/format.rst

stefanv · 2024-12-18T06:17:45Z

doc/format.rst

-constructor's parameters.
+constructor's parameters. While repetition is unnecessary, a docstring for
+the class constructor (``__init__``) can be added optionally to provide
+detailed initialization documentation.


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

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.

Yes:

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

And all that :)

Co-authored-by: Stefan van der Walt <[email protected]>

larsoner · 2024-12-30T16:45:15Z

numpydoc/validate.py

+        # 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:
+            cls_name = doc.code_obj.__qualname__.split(".")[0]
+            cls = getattr(importlib.import_module(doc.code_obj.__module__), cls_name)


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

larsoner · 2024-12-30T16:45:28Z

numpydoc/validate.py

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


Seems like this is a tiny bit safer

Suggested change

if doc.name.endswith("__init__") and doc.is_function_or_method:

if doc.name.endswith(".__init__") and doc.is_function_or_method:

mattgebert added 3 commits December 11, 2024 23:40

MAINT: Changed class constructor __init__ GL08 reporting to only if c…

bd64a7d

…lass docstring isn't complete

DOC: Described new class docstring and constructor GL08 interaction

998d72f

DOC: Fixed invalid classdoc reference

a43b5b9

larsoner added the type: Bug fix label Dec 16, 2024

Merge branch 'numpy:main' into GL08_Constructor_Checking

cc5fe97

larsoner approved these changes Dec 17, 2024

View reviewed changes

stefanv reviewed Dec 17, 2024

View reviewed changes

mattgebert and others added 5 commits December 18, 2024 14:08

Merge branch 'numpy:main' into GL08_Constructor_Checking

ea2c5e3

test(test_validate.py): Changed test class names to better reflect th…

3f51f53

…e conditions they're used to check

docs(validation.rst,-format.rst): Cleaned up doc references to constr…

66818ea

…uctor checking

test(test_validate.py): Extra test case for a constructor docstring w…

b22eb3b

…hen the constructor has no parameters, also modified docstring descriptions for clarity

docs(validation.rst): Changed words to better articular docstring dep…

be2bd62

…endency

stefanv approved these changes Dec 18, 2024

View reviewed changes

mattgebert and others added 2 commits December 18, 2024 22:34

doc(doc/format.rst): Grammar

296043d

Co-authored-by: Stefan van der Walt <[email protected]>

Merge branch 'main' into GL08_Constructor_Checking

e0edc18

larsoner reviewed Dec 30, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MAINT: Changed class constructor init GL08 reporting #592

MAINT: Changed class constructor init GL08 reporting #592

mattgebert commented Dec 11, 2024

larsoner commented Dec 16, 2024

mattgebert commented Dec 17, 2024 •

edited

Loading

larsoner commented Dec 17, 2024

larsoner commented Dec 17, 2024

larsoner left a comment

stefanv left a comment

stefanv Dec 17, 2024

mattgebert Dec 18, 2024

stefanv Dec 18, 2024

stefanv Dec 18, 2024

stefanv Dec 17, 2024

larsoner Dec 30, 2024

stefanv left a comment

stefanv Dec 18, 2024

mattgebert Dec 18, 2024

stefanv Dec 18, 2024

larsoner Dec 30, 2024

larsoner Dec 30, 2024

		@@ -1198,6 +1198,90 @@ def missing_whitespace_after_comma(self):
		"""


		class GoodConstructorInclusion:

	if doc.name.endswith("__init__") and doc.is_function_or_method:
	if doc.name.endswith(".__init__") and doc.is_function_or_method:

MAINT: Changed class constructor __init__ GL08 reporting #592

Are you sure you want to change the base?

MAINT: Changed class constructor __init__ GL08 reporting #592

Conversation

mattgebert commented Dec 11, 2024

larsoner commented Dec 16, 2024

mattgebert commented Dec 17, 2024 • edited Loading

format.rst -> Documenting classes

validation.rst -> Ignoring Validation Checks with Inline Comments

larsoner commented Dec 17, 2024

larsoner commented Dec 17, 2024

larsoner left a comment

Choose a reason for hiding this comment

stefanv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MAINT: Changed class constructor init GL08 reporting #592

MAINT: Changed class constructor init GL08 reporting #592

mattgebert commented Dec 17, 2024 •

edited

Loading

format.rst -> `Documenting classes`

validation.rst -> `Ignoring Validation Checks with Inline Comments`