Add support for Google-style comments #3

leandro-lucarella-frequenz · 2023-05-16T09:32:38Z

I know it is already planned, but I just wanted to create an issue so this has some visibility and people can also vote for it with a 👍 reaction for example.

We would love to replace the dead corpse of darglint with this tool :)

jsh9 · 2023-05-16T09:59:47Z

Hi @leandro-lucarella-frequenz , thanks for the vote of confidence!

I already found this docstring parser (https://github.com/rr-/docstring_parser) that seemed to parse Google-style docstrings well.

I plan to start to integrate this parser in a few days, if not sooner.

jsh9 · 2023-05-19T06:27:45Z

Hi @leandro-lucarella-frequenz , I've added support for the Google style. It's in 0.0.3.

Please feel free to give it a try!

I'm closing this issue as "done". But if you find any issues, bugs, or have some feature requests, please feel free to open new issues.

leandro-lucarella-frequenz · 2023-05-22T07:14:49Z

Amazing, thanks! I'll try to find a time slot to try it this week!

leandro-lucarella-frequenz · 2023-05-22T07:25:56Z

Actually I just gave it a quick try. We are using a style that includes documentation in the __init__() function (not sure if it is fully compatible with google-style) which is not checked by darglint. Is there a way to disable some checks?

For more context, we are using mkdocs + mkdocstrings to generate the docs (for example https://frequenz-floss.github.io/frequenz-channels-python/). It does have an option to mix the the class and __init__ docs, but I think you need to include some of the docs in the __init__ docstring, like the Args:, Returns:, etc. I know I tried the combined class and init` doc but found some issues with it, which made us decide for having them separated.

jsh9 · 2023-05-22T08:19:47Z

One of the current rules in pydoclint is:

DOC301: __init__() should not have a docstring; please combine it with the docstring of the class

I set up this rule because:

Sphinx (what I use to generate docs) reads the class docstring, rather than docstring under __init__()
The numpy docstyle guide suggests to document __init__() under the class name (class MyClass:)
The Google docstring style template also document the constructor under the class name

Does your team have a strong reason to put documentation in both __init__() and the class? If not, could you try to put all the docstrings under the class? If so, could you show me a code example and I can potentially add this as a configurable option of pydoclint.

leandro-lucarella-frequenz · 2023-05-22T09:22:02Z

Does your team have a strong reason to put documentation in both __init__() and the class? If not, could you try to put all the docstrings under the class? If so, could you show me a code example and I can potentially add this as a configurable option of pydoclint.

As I said, there was a reason, I don't remember exactly what, but I remember trying both out and deciding for the split. I see the template you link is from Sphinx, but the official google style guide documents the __init__ separately: https://google.github.io/styleguide/pyguide.html#384-classes

I think a mechanism to ignore some rule as most linting tools have will be generally beneficial regardless of this particular issue.

leandro-lucarella-frequenz · 2023-05-22T09:25:43Z

BTW, the Sphinx template you link also documents the __init__ in one example, so maybe that check should be completely removed when using Google-style :)

class ExampleClass:
    """The summary line for a class docstring should fit on one line.

    If the class has public attributes, they may be documented here
    in an ``Attributes`` section and follow the same formatting as a
    function's ``Args`` section. Alternatively, attributes may be documented
    inline with the attribute's declaration (see __init__ method below).

    Properties created with the ``@property`` decorator should be documented
    in the property's getter method.

    Attributes:
        attr1 (str): Description of `attr1`.
        attr2 (:obj:`int`, optional): Description of `attr2`.

    """

    def __init__(self, param1, param2, param3):
        """Example of docstring on the __init__ method.

        The __init__ method may be documented in either the class level
        docstring, or as a docstring on the __init__ method itself.

        Either form is acceptable, but the two should not be mixed. Choose one
        convention to document the __init__ method and be consistent with it.

        Note:
            Do not include the `self` parameter in the ``Args`` section.

        Args:
            param1 (str): Description of `param1`.
            param2 (:obj:`int`, optional): Description of `param2`. Multiple
                lines are supported.
            param3 (list(str)): Description of `param3`.

        """

jsh9 · 2023-05-23T09:36:41Z

Thanks for providing the code example!

Can you confirm that these are the desired behaviors:

In the __init__() docstring, check the args (param1, param2, ...) against the arguments in __init__()
In the ExampleClass docstring, check the attributes against all the self.XXX = ... in the __init__() method

The 1st point be easily achieved.

The 2nd point may be a bit tricky, because it requires the following:

Go through the actual code contents of __init__()
Find all the self.XXX = ... statements
Exclude the private attributes (self._YYY = ...)

It's probably still doable though.

leandro-lucarella-frequenz · 2023-05-23T15:47:37Z

Is correct.
Might be nice to have, but we actually don't use attributes, as mkdocstrings render them only in the class documentation but doesn't add them to the TOC. mkdocstrings accept also docstrings for instance attributes inside the __init__ body and this is what we use:

class ExampleClass:
    """The summary line for a class docstring should fit on one line.

    If the class has public attributes, they may be documented here
    in an ``Attributes`` section and follow the same formatting as a
    function's ``Args`` section. Alternatively, attributes may be documented
    inline with the attribute's declaration (see __init__ method below).

    Properties created with the ``@property`` decorator should be documented
    in the property's getter method.
    """

    class_variable: str = "test"
    """This is the documentation of the class variable."""

    def __init__(self, param1, param2, param3):
        """Example of docstring on the __init__ method.

        Args:
            ...
        """
        self.attr1: str = param1
        """This is the documentation for the attr1 instance attribute."""

I think this is pretty non-standard, and also there isn't much to check with this way of documenting, so for our specific use case 2. is not needed at all.

If I may derail a bit, speaking of attributes, one thing that is very annoying for us about darglint is also that it will check @propertys as if they were regular functions, so we are forced to duplicate the docs for properties basically because it requires a Returns:. For example:

    @property
    def delay_tolerance(self) -> timedelta:
        """Return the maximum delay that is tolerated before starting to drift.

        Returns:
            The maximum delay that is tolerated before starting to drift.
        """
        return timedelta(microseconds=self._tolerance)

We would love to write this instead:

    @property
    def delay_tolerance(self) -> timedelta:
        """The maximum delay that is tolerated before starting to drift."""
        return timedelta(microseconds=self._tolerance)

(we still would have one issue with this because we use also https://www.pydocstyle.org/en/stable/, which also requires function to start with a verb, but we didn't investigate if there is a way to escape that for properties in pydocstyle yet)

leandro-lucarella-frequenz · 2023-05-23T15:49:04Z

For reference this is the darglint issue: terrencepreilly/darglint#94

jsh9 · 2023-05-24T10:16:53Z

Hi @leandro-lucarella-frequenz :

I've merged #7, which added an option --allow-init-docstring (or -aid). I haven't released a new version yet, because I have some other code changes in mind. But you can install the latest version on main, or install it locally to try it out.

By default, it's False. If you set it to True, you can have pydoclint check the docstring of __init__(). Please see the new violation codes: DOC303 to DOC307.

As to the issue of @property: with pydoclint, you don't need to add a "return" section in your docstring if the option --skip-checking-short-docstrings is True. You can take a look at how pydoclint itself documents @property methods:

pydoclint/pydoclint/utils/arg.py

Lines 108 to 121 in 30eedbd

 @property 

 def isEmpty(self) -> bool: 

 """Whether the arg list is empty""" 

 return self.length == 0 

 @property 

 def nonEmpty(self) -> bool: 

 """Whether the arg list is non-empty""" 

 return not self.isEmpty 

 @property 

 def length(self) -> int: 

 """Calculate the length of the list""" 

 return len(self.infoList)

This is a happy coincidence. But if you for some reason need to set it to False, you run into the same issue again. Therefore I may make a code change soon to exempt @property methods for having to add a "returns" section.

But other aspects are out of the control of pydoclint:

pydocstyle requires a docstring in public methods (@property methods usually are public)
pydocstyle requires the docstring of a method/function to start with a verb

leandro-lucarella-frequenz · 2023-05-24T11:27:56Z

Hi @jsh9, first of all I want to thank you for following up on this (for so song and so deep :).

I'll try --allow-init-docstring as soon as I can. Out of curiosity, is there any reason you prefer to add individual command line options for configuration instead of having a more general mechanism to disable certain checks as most other linting tools do?

About properties, this for use would be the best option:

Therefore I may make a code change soon to exempt @Property methods for having to add a "returns" section.

We don't want to allow short descriptions in general, if a function has arguments we want to get lint error on functions that don't document them even if the docstring is one line.

pydocstyle requires the docstring of a method/function to start with a verb

Yeah, I hope at some point there is a way to change that in pydocstyle, but the worse issue we have now with properties is having to duplicate the documentation, we can live with a slightly weird description for a property that starts with a verb.

Thanks a lot again!

leandro-lucarella-frequenz · 2023-05-24T11:44:16Z

OK, I just tried it out and it works like a charm with -aid True! Also, I guess pydocstyle was updated because I also tried documenting a property without a verb and it also works fine 🎉

There are a few bad news though: After trying it I realized it worked without specifying the --style=google, so I tried adding that but then when trying --style=google I actually get a backtrace:

pydoclint -q -aid True --style=google src/
Traceback (most recent call last):
  File "/home/luca/devel/sdk/.direnv/python-3.11.2/bin/pydoclint", line 8, in <module>
    sys.exit(main())
             ^^^^^^
  File "/home/luca/devel/sdk/.direnv/python-3.11.2/lib/python3.11/site-packages/click/core.py", line 1130, in __call__
    return self.main(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/luca/devel/sdk/.direnv/python-3.11.2/lib/python3.11/site-packages/click/core.py", line 1055, in main
    rv = self.invoke(ctx)
         ^^^^^^^^^^^^^^^^
  File "/home/luca/devel/sdk/.direnv/python-3.11.2/lib/python3.11/site-packages/click/core.py", line 1404, in invoke
    return ctx.invoke(self.callback, **ctx.params)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/luca/devel/sdk/.direnv/python-3.11.2/lib/python3.11/site-packages/click/core.py", line 760, in invoke
    return __callback(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/luca/devel/sdk/.direnv/python-3.11.2/lib/python3.11/site-packages/click/decorators.py", line 26, in new_func
    return f(get_current_context(), *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/luca/devel/sdk/.direnv/python-3.11.2/lib/python3.11/site-packages/pydoclint/main.py", line 144, in main
    violationsInAllFiles: Dict[str, List[Violation]] = _checkPaths(
                                                       ^^^^^^^^^^^^
  File "/home/luca/devel/sdk/.direnv/python-3.11.2/lib/python3.11/site-packages/pydoclint/main.py", line 225, in _checkPaths
    violationsInThisFile: List[Violation] = _checkFile(
                                            ^^^^^^^^^^^
  File "/home/luca/devel/sdk/.direnv/python-3.11.2/lib/python3.11/site-packages/pydoclint/main.py", line 260, in _checkFile
    visitor.visit(tree)
  File "/usr/lib/python3.11/ast.py", line 418, in visit
    return visitor(node)
           ^^^^^^^^^^^^^
  File "/usr/lib/python3.11/ast.py", line 426, in generic_visit
    self.visit(item)
  File "/usr/lib/python3.11/ast.py", line 418, in visit
    return visitor(node)
           ^^^^^^^^^^^^^
  File "/home/luca/devel/sdk/.direnv/python-3.11.2/lib/python3.11/site-packages/pydoclint/visitor.py", line 51, in visit_ClassDef
    self.generic_visit(node)
  File "/usr/lib/python3.11/ast.py", line 426, in generic_visit
    self.visit(item)
  File "/usr/lib/python3.11/ast.py", line 418, in visit
    return visitor(node)
           ^^^^^^^^^^^^^
  File "/home/luca/devel/sdk/.direnv/python-3.11.2/lib/python3.11/site-packages/pydoclint/visitor.py", line 102, in visit_FunctionDef
    returnViolations = self.checkReturns(node, parent_, doc)
                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/luca/devel/sdk/.direnv/python-3.11.2/lib/python3.11/site-packages/pydoclint/visitor.py", line 358, in checkReturns
    hasGenAsRetAnno: bool = hasGeneratorAsReturnAnnotation(node)
                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/luca/devel/sdk/.direnv/python-3.11.2/lib/python3.11/site-packages/pydoclint/utils/return_yield_raise.py", line 25, in hasGeneratorAsReturnAnnotation
    returnAnnotation: str = parseAnnotation(node.returns)
                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/luca/devel/sdk/.direnv/python-3.11.2/lib/python3.11/site-packages/pydoclint/utils/annotation.py", line 17, in parseAnnotation
    slice_: str = parseAnnotation(node.slice)
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/luca/devel/sdk/.direnv/python-3.11.2/lib/python3.11/site-packages/pydoclint/utils/annotation.py", line 24, in parseAnnotation
    return ', '.join(map(parseAnnotation, node.elts))
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: sequence item 0: expected str instance, NoneType found

Also trying to use -scsd False gives the exact same backtrace.

PS: Using pip install git+https://github.com/jsh9/pydoclint.git to install pydoclint and at the time of writing that is 30eedbd.

jsh9 · 2023-05-25T07:48:49Z

Let me take a look at this bug.

jsh9 · 2023-05-25T07:54:36Z

Would you be able to isolate the Python file that resulted in this issue? If so, I'd be able to isolate the issue much easier. Right now it's a bit hard for me to reproduce this issue on my end.

jsh9 · 2023-05-25T08:02:05Z

I found the file that triggered this issue. It's this one: https://github.com/frequenz-floss/frequenz-channels-python/blob/ef9bc4740b8e9d1d0a51b109089832a4acc4fdeb/src/frequenz/channels/_base_classes.py

Let me debug it.

jsh9 · 2023-05-25T08:35:57Z

I fixed the bug in this commit: 64f6250

And then I tested pydoclint on your repo (https://github.com/frequenz-floss/frequenz-channels-python) with this command:

pydoclint -aid True --style=google src/

It worked well.

leandro-lucarella-frequenz · 2023-05-25T09:45:05Z

Oh, amazing, thanks for digging into this with so little info provided!

I tried it with another project we have and I'm still getting the exception (or a very similar one):

$ pydoclint -aid True --style=google src/
Skipping files that match this pattern: \.git|\.tox
src/conftest.py
...
src/frequenz/sdk/timeseries/_ringbuffer/serialization.py
Traceback (most recent call last):
  File "/home/luca/devel/sdk/.direnv/python-3.11.2/bin/pydoclint", line 8, in <module>
    sys.exit(main())
             ^^^^^^
  File "/home/luca/devel/sdk/.direnv/python-3.11.2/lib/python3.11/site-packages/click/core.py", line 1130, in __call__
    return self.main(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/luca/devel/sdk/.direnv/python-3.11.2/lib/python3.11/site-packages/click/core.py", line 1055, in main
    rv = self.invoke(ctx)
         ^^^^^^^^^^^^^^^^
  File "/home/luca/devel/sdk/.direnv/python-3.11.2/lib/python3.11/site-packages/click/core.py", line 1404, in invoke
    return ctx.invoke(self.callback, **ctx.params)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/luca/devel/sdk/.direnv/python-3.11.2/lib/python3.11/site-packages/click/core.py", line 760, in invoke
    return __callback(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/luca/devel/sdk/.direnv/python-3.11.2/lib/python3.11/site-packages/click/decorators.py", line 26, in new_func
    return f(get_current_context(), *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/luca/devel/sdk/.direnv/python-3.11.2/lib/python3.11/site-packages/pydoclint/main.py", line 144, in main
    violationsInAllFiles: Dict[str, List[Violation]] = _checkPaths(
                                                       ^^^^^^^^^^^^
  File "/home/luca/devel/sdk/.direnv/python-3.11.2/lib/python3.11/site-packages/pydoclint/main.py", line 225, in _checkPaths
    violationsInThisFile: List[Violation] = _checkFile(
                                            ^^^^^^^^^^^
  File "/home/luca/devel/sdk/.direnv/python-3.11.2/lib/python3.11/site-packages/pydoclint/main.py", line 260, in _checkFile
    visitor.visit(tree)
  File "/usr/lib/python3.11/ast.py", line 418, in visit
    return visitor(node)
           ^^^^^^^^^^^^^
  File "/usr/lib/python3.11/ast.py", line 426, in generic_visit
    self.visit(item)
  File "/usr/lib/python3.11/ast.py", line 418, in visit
    return visitor(node)
           ^^^^^^^^^^^^^
  File "/home/luca/devel/sdk/.direnv/python-3.11.2/lib/python3.11/site-packages/pydoclint/visitor.py", line 88, in visit_FunctionDef
    doc: Doc = Doc(docstring=docstring, style=self.style)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/luca/devel/sdk/.direnv/python-3.11.2/lib/python3.11/site-packages/pydoclint/utils/doc.py", line 26, in __init__
    self.parsed = parser.parse(docstring)
                  ^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/luca/devel/sdk/.direnv/python-3.11.2/lib/python3.11/site-packages/docstring_parser/google.py", line 285, in parse
    ret.meta.append(self._build_meta(part, title))
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/luca/devel/sdk/.direnv/python-3.11.2/lib/python3.11/site-packages/docstring_parser/google.py", line 112, in _build_meta
    raise ParseError(f"Expected a colon in {text!r}.")
docstring_parser.common.ParseError: Expected a colon in 'I/O related exceptions when the file cannot be written.'.

This is the file triggering the issue: https://github.com/frequenz-floss/frequenz-sdk-python/blob/v0.x.x/src/frequenz/sdk/timeseries/_ringbuffer/serialization.py

jsh9 · 2023-05-26T08:00:06Z

So this is a separate issue.

At the end of the call stack:

docstring_parser.common.ParseError: Expected a colon in 'I/O related exceptions when the file cannot be written.'.

This means that this part of the docstring is not written in a way that conforms to the Google-style docstring.

I think it should be written like this:

    Raises:
        IOError: I/O related exceptions when the file cannot be written.

That said, unparsable docstrings should not have failed the entire linter. So I'm going to make a code change to turn docstring parsing errors into a style violation.

leandro-lucarella-frequenz · 2023-05-26T10:02:16Z

Ah, interesting that darglint didn't caught that! Yeah, it would be great if the tool didn't crash on these kind of parsing errors and it would be awesome if they are somewhat reported to the user :)

Thanks again for looking into it!

jsh9 · 2023-05-27T10:30:55Z

Hi @leandro-lucarella-frequenz , I just published version 0.0.4.

In this version, you'll see this violation:

src/frequenz/sdk/timeseries/_ringbuffer/serialization.py:48:1: 
DOC001: Function/method `dump`: Potential formatting errors in docstring. Error 
message: Expected a colon in 'I/O related exceptions when the file cannot be written.'.

jsh9 · 2023-05-31T10:46:03Z

Hi @leandro-lucarella-frequenz : I made a code change (#13) to address the issue that @property methods need a return section.

With this code change, property methods no longer need a return section in their docstrings.

leandro-lucarella-frequenz · 2023-06-01T07:58:30Z

Hi @jsh9, thanks a lot for the updates!

I'm still bumping into a few issues, but I think I already hijacked this issue enough, so I'm creating new issues for the other problems :)

jsh9 mentioned this issue May 17, 2023

Support Google-style docstring #5

Merged

jsh9 closed this as completed May 19, 2023

leandro-lucarella-frequenz mentioned this issue May 22, 2023

Replace darglint with ruff or pydoclint frequenz-floss/frequenz-repo-config-python#18

Closed

jsh9 mentioned this issue May 24, 2023

Add option to allow __init__() to have docstring #7

Merged

jsh9 mentioned this issue May 27, 2023

Add docstring parsing errors as DOC001 #8

Merged

jsh9 mentioned this issue May 31, 2023

Allow no return section in property methods #13

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Google-style comments #3

Add support for Google-style comments #3

leandro-lucarella-frequenz commented May 16, 2023

jsh9 commented May 16, 2023

jsh9 commented May 19, 2023

leandro-lucarella-frequenz commented May 22, 2023

leandro-lucarella-frequenz commented May 22, 2023

jsh9 commented May 22, 2023 •

edited

Loading

leandro-lucarella-frequenz commented May 22, 2023 •

edited

Loading

leandro-lucarella-frequenz commented May 22, 2023 •

edited

Loading

jsh9 commented May 23, 2023

leandro-lucarella-frequenz commented May 23, 2023 •

edited

Loading

leandro-lucarella-frequenz commented May 23, 2023

jsh9 commented May 24, 2023 •

edited

Loading

leandro-lucarella-frequenz commented May 24, 2023

leandro-lucarella-frequenz commented May 24, 2023 •

edited

Loading

jsh9 commented May 25, 2023

jsh9 commented May 25, 2023

jsh9 commented May 25, 2023

jsh9 commented May 25, 2023

leandro-lucarella-frequenz commented May 25, 2023

jsh9 commented May 26, 2023

leandro-lucarella-frequenz commented May 26, 2023

jsh9 commented May 27, 2023

jsh9 commented May 31, 2023

leandro-lucarella-frequenz commented Jun 1, 2023

Add support for Google-style comments #3

Add support for Google-style comments #3

Comments

leandro-lucarella-frequenz commented May 16, 2023

jsh9 commented May 16, 2023

jsh9 commented May 19, 2023

leandro-lucarella-frequenz commented May 22, 2023

leandro-lucarella-frequenz commented May 22, 2023

jsh9 commented May 22, 2023 • edited Loading

leandro-lucarella-frequenz commented May 22, 2023 • edited Loading

leandro-lucarella-frequenz commented May 22, 2023 • edited Loading

jsh9 commented May 23, 2023

leandro-lucarella-frequenz commented May 23, 2023 • edited Loading

leandro-lucarella-frequenz commented May 23, 2023

jsh9 commented May 24, 2023 • edited Loading

leandro-lucarella-frequenz commented May 24, 2023

leandro-lucarella-frequenz commented May 24, 2023 • edited Loading

jsh9 commented May 25, 2023

jsh9 commented May 25, 2023

jsh9 commented May 25, 2023

jsh9 commented May 25, 2023

leandro-lucarella-frequenz commented May 25, 2023

jsh9 commented May 26, 2023

leandro-lucarella-frequenz commented May 26, 2023

jsh9 commented May 27, 2023

jsh9 commented May 31, 2023

leandro-lucarella-frequenz commented Jun 1, 2023

jsh9 commented May 22, 2023 •

edited

Loading

leandro-lucarella-frequenz commented May 22, 2023 •

edited

Loading

leandro-lucarella-frequenz commented May 22, 2023 •

edited

Loading

leandro-lucarella-frequenz commented May 23, 2023 •

edited

Loading

jsh9 commented May 24, 2023 •

edited

Loading

leandro-lucarella-frequenz commented May 24, 2023 •

edited

Loading