Check GL08 on class __init__ constructors

[mattgebert]

I was looking for a way to skip GL08 checks on class constructors (__init__) throughout my code. This is particularly as the numpydoc recommendation for class strings documents the constructor:

Class docstring
Use the same sections as outlined above (all except Returns are applicable). The constructor (__init__) should also be documented here, the Parameters section of the docstring details the constructor’s parameters.

It seems that there ought to be a default check ignore for __init__ constructors if no docstring has been provided (and if a class docstring has been provided). Obviously if an (optional) docstring for __init__ has been created, then it ought to be checked.

This conditional checking doesn't seem possible with the default override_<code>, exclude toml tags.
Keen to hear others thoughts!

[mattgebert]

In case it wasn't clear, GL08 has the description "The object does not have a docstring".

[stefanv]

That makes sense to me.

[mattgebert]

Hey all (@larsoner, @stefanv),

I have re-opened the issue as the current implementation is not working with the AstValidator. It is my understanding that the AstValidator is the preferred validator class, used when running numpydoc linting and CI testing? (i.e. The modifications we made are not /will not be used in the majority of numpydoc validation).

When we improved the class constructor init GL08 reporting, we only wrote tests for the Validator class which works perfectly. This is because Validator uses the inspect library to find the code_obj, we can determine the parent module / class to check if the __init__ is documented in the constructor.

However, under the implementation of the abstract syntax tree (ast) validator (numpydoc.hooks.validate_docstrings.AstValidator) the code_obj attribute does not become defined, making the resolving of the parent class (that the constructor is defined on) difficult. I have done a preliminary check if I can still resolve the similar names required to run the Validator._load_obj without the code_obj attribute.

Here's the way we get currently (roughly) get the class reference from the constructor:

if hasattr(doc, "code_obj"):
    cls_name = doc.code_obj.__qualname__.split(".")[0]
    module_name = doc.code_obj.__module__
    cls = Validator._load_obj(f"{module_name}.{cls_name}")

Any ideas what we should do for the AstValidator? Unfortunately it seems that the ast nodes, including ast.FunctionDef, only have a body attribute to see sub-nodes, but there's no link to the parent node.

The AstValidator is given a _filename attribute and _name attribute. The latter is a concat list of the sub-module class nodes, which should allow us to track to the class structure. I've just tested the following to be working:

elif isinstance(doc, AstValidator):
    # Equivalent to the above, but for the AstValidator in validate_docstrings.py.
    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)
        
        # Create a function to find the class node
        def find_class_node(node: ast.AST, cls_name) -> ast.ClassDef:
            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}")
        
        # Recursively find each subclass
        next_node = module_node
        for name in nested_cls_names:
            next_node = find_class_node(next_node, name)
        
        # Get the documentation.
        assert isinstance(next_node, ast.ClassDef)
        assert next_node.name == cls_name
        cls_doc = AstValidator(ast_node=next_node, filename=filename, obj_name=cls_name)
    # else:
    #     # Ignore module.__init__ edge case functions. 

This seems quite convoluted just to get the parent class for the constructor. What are your thoughts?

[mattgebert]

Further discussion on this, should all tests in test_validate also be written/run with AstValidator in mind?

[mattgebert]

@stefanv Is it just me or are we not properly testing hook validation at all? I searched the run test suite CI for references to hook (i.e. numpydoc/tests/hooks/test_validate_hook.py) but I don't believe test.yml actually runs this folder subset.

I also wonder if our hook tests ought to be consistent with the regular tests, particularly given the unique AST traversal? Wondering if we can consolidate tests into some example files so both AST and regular Validator functionality can be routinely verified?

(edit) Could #556 be the reason and affecting our CI testing as well?

[stefanv]

@mattgebert We run the tests using two different invocations:

pytest -v --pyargs numpydoc

and

pytest -v --pyargs .

The latter happens on the prerelease job, and does execute the hooks tests.

BTW, the naming of that job made perfect sense when @jarrodmillman added it. But, it looks like it has been modified in 342bdce not to install pre-releases, so now makes less sense. We should probably re-introduce the --pre flag there, and also unify the way we invoke the tests (pytest -v . or similar), to ensure that the hook tests are run.

I haven't looked at the AST vs regular validator for a long time, and the details escape me now. I thought the AST validator did additional tests on top of the docstring validation (such as whether all arguments are documented). Ideally, we then shouldn't need to test the same functionality twice, once in the Validator and another time in ASTValidator?

numpydoc needs much love, so your input is much appreciated.

[mattgebert]

Sorry it's been a while. I've added (#622) a fix and updated test to ensure the AST Validation functionality is also checked.

We should probably re-introduce the --pre flag there, and also unify the way we invoke the tests (pytest -v . or similar), to ensure that the hook tests are run.

Sounds good, let's fix this up and add some extra commits to #622 to ensure this is on track.

I thought the AST validator did additional tests on top of the docstring validation (such as whether all arguments are documented). Ideally, we then shouldn't need to test the same functionality twice, once in the Validator and another time in ASTValidator?

I don't believe there are any additional validation checks, but there are tests for the use of configs (toml, setup) which seem useful. (hook testing goes like: hooks.validate_docstrings.py the run_hook which simply creates DocstringVisitor instances for each file. The visit method simply runs the validate method in validate.py, and everything runs pretty simple imo.)

I get the impression that the many docstring examples in test_validate.py, and the example_module.py (from hook tests) could be consolidated at some point but this would be a big job to update the testing suite such that examples (good and bad) were relocated, and all test modules point to such examples to run their comprehensive tests.

For the meantime, I've modified the example_module.py (tested by test_validate_hook.py) to reflect the required testing from the GL08 changes. I think the testing in test_validate_hook.py is sufficient, though certainly less comprehensive than the regular Validator.