Skip to content

Constructor checking for AST validator #622

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open
5 changes: 3 additions & 2 deletions .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ jobs:
- name: Run test suite
run: |
pytest -v --pyargs numpydoc
pytest -v --pyargs numpydoc/tests/hooks

- name: Test coverage
run: |
Expand Down Expand Up @@ -95,12 +96,12 @@ jobs:

- name: Install
run: |
python -m pip install .[test,doc]
python -m pip install --pre .[test,doc]
pip list

- name: Run test suite
run: |
pytest -v --pyargs .
pytest -v --pyargs numpydoc

- name: Test coverage
run: |
Expand Down
1 change: 0 additions & 1 deletion numpydoc/tests/hooks/__init__.py

This file was deleted.

33 changes: 32 additions & 1 deletion numpydoc/tests/hooks/example_module.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,4 +28,35 @@ def create(self):


class NewClass:
pass
class GoodConstructor:
"""
A nested class to test constructors via AST hook.

Implements constructor via class docstring.

Parameters
----------
name : str
The name of the new class.
"""

def __init__(self, name):
self.name = name

class BadConstructor:
"""
A nested class to test constructors via AST hook.

Implements a bad constructor docstring despite having a good class docstring.

Parameters
----------
name : str
The name of the new class.
"""

def __init__(self, name):
"""
A failing constructor implementation without parameters.
"""
self.name = name
38 changes: 28 additions & 10 deletions numpydoc/tests/hooks/test_validate_hook.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
"""Test the numpydoc validate pre-commit hook."""

import inspect
import os
from pathlib import Path

import pytest
Expand Down Expand Up @@ -40,8 +41,6 @@ def test_validate_hook(example_module, config, capsys):

numpydoc/tests/hooks/example_module.py:8: EX01 No examples section found

numpydoc/tests/hooks/example_module.py:11: GL08 The object does not have a docstring

numpydoc/tests/hooks/example_module.py:17: ES01 No extended summary found

numpydoc/tests/hooks/example_module.py:17: PR01 Parameters {'**kwargs'} not documented
Expand All @@ -61,8 +60,24 @@ def test_validate_hook(example_module, config, capsys):
numpydoc/tests/hooks/example_module.py:26: EX01 No examples section found

numpydoc/tests/hooks/example_module.py:30: GL08 The object does not have a docstring

numpydoc/tests/hooks/example_module.py:31: SA01 See Also section not found

numpydoc/tests/hooks/example_module.py:31: EX01 No examples section found

numpydoc/tests/hooks/example_module.py:46: SA01 See Also section not found

numpydoc/tests/hooks/example_module.py:46: EX01 No examples section found

numpydoc/tests/hooks/example_module.py:58: ES01 No extended summary found

numpydoc/tests/hooks/example_module.py:58: PR01 Parameters {'name'} not documented

numpydoc/tests/hooks/example_module.py:58: SA01 See Also section not found

numpydoc/tests/hooks/example_module.py:58: EX01 No examples section found
"""
)
).replace("/", os.sep)

return_code = run_hook([example_module], config=config)
assert return_code == 1
Expand All @@ -79,17 +94,17 @@ def test_validate_hook_with_ignore(example_module, capsys):
"""
numpydoc/tests/hooks/example_module.py:4: PR01 Parameters {'name'} not documented

numpydoc/tests/hooks/example_module.py:11: GL08 The object does not have a docstring

numpydoc/tests/hooks/example_module.py:17: PR01 Parameters {'**kwargs'} not documented

numpydoc/tests/hooks/example_module.py:17: PR07 Parameter "*args" has no description

numpydoc/tests/hooks/example_module.py:26: SS05 Summary must start with infinitive verb, not third person (e.g. use "Generate" instead of "Generates")

numpydoc/tests/hooks/example_module.py:30: GL08 The object does not have a docstring

numpydoc/tests/hooks/example_module.py:58: PR01 Parameters {'name'} not documented
"""
)
).replace("/", os.sep)

return_code = run_hook([example_module], ignore=["ES01", "SA01", "EX01"])

Expand Down Expand Up @@ -132,7 +147,7 @@ def test_validate_hook_with_toml_config(example_module, tmp_path, capsys):

numpydoc/tests/hooks/example_module.py:30: GL08 The object does not have a docstring
"""
)
).replace("/", os.sep)

return_code = run_hook([example_module], config=tmp_path)
assert return_code == 1
Expand Down Expand Up @@ -167,7 +182,7 @@ def test_validate_hook_with_setup_cfg(example_module, tmp_path, capsys):

numpydoc/tests/hooks/example_module.py:30: GL08 The object does not have a docstring
"""
)
).replace("/", os.sep)

return_code = run_hook([example_module], config=tmp_path)
assert return_code == 1
Expand Down Expand Up @@ -208,7 +223,7 @@ def test_validate_hook_exclude_option_pyproject(example_module, tmp_path, capsys

numpydoc/tests/hooks/example_module.py:30: GL08 The object does not have a docstring
"""
)
).replace("/", os.sep)

return_code = run_hook([example_module], config=tmp_path)
assert return_code == 1
Expand Down Expand Up @@ -241,8 +256,11 @@ def test_validate_hook_exclude_option_setup_cfg(example_module, tmp_path, capsys

numpydoc/tests/hooks/example_module.py:17: PR07 Parameter "*args" has no description
"""
)
).replace("/", os.sep)

return_code = run_hook([example_module], config=tmp_path)
assert return_code == 1
assert capsys.readouterr().err.strip() == expected


# def test_validate_hook_
80 changes: 80 additions & 0 deletions numpydoc/tests/test_validate.py
Original file line number Diff line number Diff line change
Expand Up @@ -1305,6 +1305,69 @@ def __init__(self, param1: int):
pass


class ConstructorDocumentedinEmbeddedClass: # ignore Gl08, ES01
"""
Class to test the initialisation behaviour of a embedded class.
"""

class EmbeddedClass1: # ignore GL08, ES01
"""
An additional level for embedded class documentation checking.
"""

class EmbeddedClass2:
"""
This is an embedded class.

Extended summary.

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

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

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

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


class IncompleteConstructorDocumentedinEmbeddedClass:
"""
Class to test the initialisation behaviour of a embedded class.
"""

class EmbeddedClass1:
"""
An additional level for embedded class documentation checking.
"""

class EmbeddedClass2:
"""
This is an embedded class.

Extended summary.

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

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

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


class TestValidator:
def _import_path(self, klass=None, func=None):
"""
Expand Down Expand Up @@ -1660,6 +1723,18 @@ def test_bad_docstrings(self, capsys, klass, func, msgs):
tuple(),
("PR01"), # Parameter not documented in class constructor
),
(
"ConstructorDocumentedinEmbeddedClass.EmbeddedClass1.EmbeddedClass2",
tuple(),
("GL08",),
tuple(),
),
(
"IncompleteConstructorDocumentedinEmbeddedClass.EmbeddedClass1.EmbeddedClass2",
("GL08",),
tuple(),
("PR01",),
),
],
)
def test_constructor_docstrings(
Expand All @@ -1677,6 +1752,11 @@ def test_constructor_docstrings(
for code in exc_init_codes:
assert code not in " ".join(err[0] for err in result["errors"])

if klass == "ConstructorDocumentedinEmbeddedClass":
raise NotImplementedError(
"Test for embedded class constructor docstring not implemented yet."
)


def decorator(x):
"""Test decorator."""
Expand Down
59 changes: 49 additions & 10 deletions numpydoc/validate.py
Original file line number Diff line number Diff line change
Expand Up @@ -572,6 +572,14 @@ def _check_desc(desc, code_no_desc, code_no_upper, code_no_period, **kwargs):
return errs


def _find_class_node(module_node: ast.AST, cls_name) -> ast.ClassDef:
# Find the class node within a module, when checking constructor docstrings.
for node in ast.walk(module_node):
if isinstance(node, ast.ClassDef) and node.name == cls_name:
return node
raise ValueError(f"Could not find class node {cls_name}")


def validate(obj_name, validator_cls=None, **validator_kwargs):
"""
Validate the docstring.
Expand Down Expand Up @@ -639,20 +647,51 @@ def validate(obj_name, validator_cls=None, **validator_kwargs):
report_GL08: bool = True
# Check if the object is a class and has a docstring in the constructor
# Also check if code_obj is defined, as undefined for the AstValidator in validate_docstrings.py.
if (
doc.name.endswith(".__init__")
and doc.is_function_or_method
and hasattr(doc, "code_obj")
):
cls_name = doc.code_obj.__qualname__.split(".")[0]
cls = Validator._load_obj(f"{doc.code_obj.__module__}.{cls_name}")
# cls = Validator._load_obj(f"{doc.name[:-9]}.{cls_name}") ## Alternative
cls_doc = Validator(get_doc_object(cls))
if doc.name.endswith(".__init__") and doc.is_function_or_method:
from numpydoc.hooks.validate_docstrings import (
AstValidator, # Support abstract syntax tree hook.
)

if hasattr(doc, "code_obj"):
cls_name = ".".join(
doc.code_obj.__qualname__.split(".")[:-1]
) # Collect all class depths before the constructor.
cls = Validator._load_obj(f"{doc.code_obj.__module__}.{cls_name}")
# cls = Validator._load_obj(f"{doc.name[:-9]}.{cls_name}") ## Alternative
cls_doc = Validator(get_doc_object(cls))
elif isinstance(doc, AstValidator): # Supports class traversal for ASTs.
names = doc._name.split(".")

if len(names) > 2: # i.e. module.class.__init__
nested_cls_names = names[1:-1] # class1,class2 etc.
cls_name = names[-2]
filename = doc.source_file_name # from the AstValidator object

# Use AST to find the class node...
with open(filename) as file:
module_node = ast.parse(file.read(), filename)

# Recursively find each subclass from the module node.
next_node = module_node
for name in nested_cls_names:
next_node = _find_class_node(next_node, name)
# Get the documentation.
cls_doc = AstValidator(
ast_node=next_node, filename=filename, obj_name=cls_name
)
else:
# Ignore edge case: __init__ functions that don't belong to a class.
cls_doc = None
Comment on lines +650 to +684
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is the way to do this. The DocstringVisitor class is already traversing the tree and capturing node ancestry (currently, just the name, but after #623, it will be all AST node ancestors). This is then used to instantiate the AstValidator, so we should be able to use that information to modify the check for what you are trying to achieve. A lot of the logic here is pretty much doing what those classes are doing, but for a second time, which I would like to avoid.

You may be able to simply filter out any cases that shouldn't be flagged in DocstringVisitor._get_numpydoc_issues(), at which point you would have access to the AST node and its ancestry. This is how the ignore rules are applied for the hook.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Brilliant - I was actually hoping this could be the case prior to making these changes.

Once #623 is merged I'll be happy update this logic and use the ancestry.

else:
raise TypeError(
f"Cannot load {doc.name} as a usable Validator object (Validator does not have `doc_obj` attr or type `AstValidator`)."
)

# 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
if cls_doc:
report_GL08 = len(cls_doc.parameter_mismatches) > 0

# Check if GL08 is to be ignored:
if "GL08" in ignore_validation_comments:
Expand Down
Loading