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

Add class attributes checking support #122

Closed
Oreilles opened this issue Feb 14, 2024 · 9 comments · Fixed by #130
Closed

Add class attributes checking support #122

Oreilles opened this issue Feb 14, 2024 · 9 comments · Fixed by #130

Comments

@Oreilles
Copy link

It would be great if it was possible, in addition to checking function docstrings, to also check for missordered, missing, unexpected, or inconsistent class attributes in docstrings. Here's a simple class example to show how class attributes can be documented in Google style:

class User:
    """Application user.

    Attributes:
        username (str): The user's name
        passwd (str): The user's password 
    """

    username: str
    password: str
@jsh9
Copy link
Owner

jsh9 commented Feb 16, 2024

Hi @Oreilles , thanks for submitting this issue.

From my perspective, the potential difficulty with this suggestion is: instance attributes are difficult to check.

Here's an example of "class attributes" and "instance attributes", in case there are any confusions:

class Car:
    # Class attribute
    num_wheels = 4
    
    def __init__(self, make, model):
        # Instance attributes
        self.make = make
        self.model = model

According to the Google style guide, if we are to document public attributes, we should document both class attributes and instance attributes.

The order and type hints of instance attributes are not easy to check (we may not want to enforce order anyways).

@Gabriel-p
Copy link
Contributor

Came here after realizing that my dataclass attributes were not being checked. Maybe an option that can be set to enforce checking class attributes with caveats shown on screen?

@jsh9
Copy link
Owner

jsh9 commented Jun 23, 2024

Hi @Gabriel-p and @Oreilles , I've implemented this functionality via a large code change. The changes are published as a new version (0.5.0).

@Gabriel-p
Copy link
Contributor

Hi @jsh9 I'm testing this new version now and I get this error:

$ pydoclint --style=sphinx .
Loading config from user-specified .toml file: pyproject.toml
No config found in pyproject.toml.
Skipping files that match this pattern: \.git|\.tox
asteca/__init__.py
asteca/cluster.py
Traceback (most recent call last):
  File "/home/gabriel/miniconda3/envs/asteca/bin/pydoclint", line 8, in <module>
    sys.exit(main())
  File "/home/gabriel/miniconda3/envs/asteca/lib/python3.10/site-packages/click/core.py", line 1157, in __call__
    return self.main(*args, **kwargs)
  File "/home/gabriel/miniconda3/envs/asteca/lib/python3.10/site-packages/click/core.py", line 1078, in main
    rv = self.invoke(ctx)
  File "/home/gabriel/miniconda3/envs/asteca/lib/python3.10/site-packages/click/core.py", line 1434, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/home/gabriel/miniconda3/envs/asteca/lib/python3.10/site-packages/click/core.py", line 783, in invoke
    return __callback(*args, **kwargs)
  File "/home/gabriel/miniconda3/envs/asteca/lib/python3.10/site-packages/click/decorators.py", line 33, in new_func
    return f(get_current_context(), *args, **kwargs)
  File "/home/gabriel/miniconda3/envs/asteca/lib/python3.10/site-packages/pydoclint/main.py", line 364, in main
    violationsInAllFiles: Dict[str, List[Violation]] = _checkPaths(
  File "/home/gabriel/miniconda3/envs/asteca/lib/python3.10/site-packages/pydoclint/main.py", line 527, in _checkPaths
    violationsInThisFile: List[Violation] = _checkFile(
  File "/home/gabriel/miniconda3/envs/asteca/lib/python3.10/site-packages/pydoclint/main.py", line 591, in _checkFile
    visitor.visit(tree)
  File "/home/gabriel/miniconda3/envs/asteca/lib/python3.10/ast.py", line 418, in visit
    return visitor(node)
  File "/home/gabriel/miniconda3/envs/asteca/lib/python3.10/ast.py", line 426, in generic_visit
    self.visit(item)
  File "/home/gabriel/miniconda3/envs/asteca/lib/python3.10/ast.py", line 418, in visit
    return visitor(node)
  File "/home/gabriel/miniconda3/envs/asteca/lib/python3.10/site-packages/pydoclint/visitor.py", line 91, in visit_ClassDef
    checkClassAttributesAgainstClassDocstring(
  File "/home/gabriel/miniconda3/envs/asteca/lib/python3.10/site-packages/pydoclint/utils/visitor_helper.py", line 39, in checkClassAttributesAgainstClassDocstring
    actualArgs: ArgList = _convertClassAttributesIntoArgList(classAttributes)
  File "/home/gabriel/miniconda3/envs/asteca/lib/python3.10/site-packages/pydoclint/utils/visitor_helper.py", line 96, in _convertClassAttributesIntoArgList
    atl.append(Arg.fromAstAnnAssign(attr))
  File "/home/gabriel/miniconda3/envs/asteca/lib/python3.10/site-packages/pydoclint/utils/arg.py", line 115, in fromAstAnnAssign
    typeHint=astAnnAssign.annotation.id,
AttributeError: 'Attribute' object has no attribute 'id'

where the error occurs is this one

@jsh9
Copy link
Owner

jsh9 commented Jun 24, 2024

Hi @Gabriel-p , thanks for testing it. It's a bug that I wrote.

I fixed it in #132 and it's published as 0.5.1.

Btw, I read your code example in https://github.com/asteca/ASteCA/blob/334e56ec57a6bd726b2bf12a3b87d9eed6131971/asteca/cluster.py, and I think you'd need to update your docstring to use .. attributes : instead of :param. You can read more about it here: https://jsh9.github.io/pydoclint/checking_class_attributes.html

@Gabriel-p
Copy link
Contributor

Gabriel-p commented Jun 24, 2024

Thank you @jsh9. I updated and that issue is gone but there seems to be a few new issue now. Running

$ pydoclint --style=sphinx .

on my package used to find lots of issues with files in the asteca/modules/ folder, now it finds no issues (and there are many). It seems to only be checking the files on the parent asteca/ folder

(ALso, minor correction in https://jsh9.github.io/pydoclint/checking_class_attributes.html: .. attribute :: weight_in_keg should be .. attribute :: weight_in_kg)

@jonyscathe
Copy link

I love this addition, but I feel like the implementation within dataclasses could be more flexible.
I somewhat feel like dataclass fields are more like parameters given that a dataclass implies an __init__ with all dataclass attributes passed in as parameters.
And if any of the dataclass fields are InitVar types then they are definitely not class attributes as InitVar fields are only used for __post_init__ functions and definitely should not be documented as an attribute (which is what I have to now do if I want to avoid DOC601/DOC603 errors).

@jsh9
Copy link
Owner

jsh9 commented Jun 25, 2024

Thank you @jsh9. I updated and that issue is gone but there seems to be a few new issue now. Running

$ pydoclint --style=sphinx .

on my package used to find lots of issues with files in the asteca/modules/ folder, now it finds no issues (and there are many). It seems to only be checking the files on the parent asteca/ folder

(ALso, minor correction in https://jsh9.github.io/pydoclint/checking_class_attributes.html: .. attribute :: weight_in_keg should be .. attribute :: weight_in_kg)

Hi @Gabriel-p , I was able to reproduce it on my end. I created #139 to track this.

@jsh9
Copy link
Owner

jsh9 commented Jun 25, 2024

I love this addition, but I feel like the implementation within dataclasses could be more flexible. I somewhat feel like dataclass fields are more like parameters given that a dataclass implies an __init__ with all dataclass attributes passed in as parameters. And if any of the dataclass fields are InitVar types then they are definitely not class attributes as InitVar fields are only used for __post_init__ functions and definitely should not be documented as an attribute (which is what I have to now do if I want to avoid DOC601/DOC603 errors).

Hi @jonyscathe : thank you for your comment!

I had thought about the same thing that you did, and I shared my thought in an earlier comment above (#122 (comment)).

If we are to treat them both class attributes and "instance attributes" as "parameters", here is what would happen:

  • People would need to document both the class attributes and "instance attributes" together, and even if people want to separate them in docstrings, pydoclint would start to complain
  • The order of parameters in the docstring becomes a bit tricky: pydoclint would enforce a particular ordering convention (such as, class attributes first) and ask people to memorize that

That's why I think distinguishing class attributes and "parameters" is an acceptable compromise. I would admit that it's not perfect, but it seems the "lesser of two evals" given Python's design.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants