Docstrings + Inheritance

[MiWeiss]

I am getting false missing documentation on methods that inherit the documentation from their superclasses. Is there any way to avoid this?

[HunterMcGushion]

@MiWeiss, thanks for your report! Sorry about the unexpected/incorrect behavior. I made a small sample file (sample_file.py) to better understand the problem:

class FooBar:
    def __init__(self):
        """This is a dunder method docstring"""

    def a_method(self):
        """This is a regular method docstring"""

class FooBaz(FooBar):
    def a_method(self):
        ...  # Override `a_method`

Running docstr-coverage --skipclassdef --skipfiledoc sample_file.py outputs (among other things)

...
- No docstring for `FooBaz.a_method`
...

Am I correct in assuming this is the behavior that's causing you problems?

I understand how this could be frustrating, but I think that if a method is being overridden, an updated docstring is probably appropriate. The above example doesn't complain about missing FooBaz.__init__ since it isn't overridden. It seems like you're probably getting the No docstring message on methods that you're overriding. Is that correct, or are you getting the missing docstring message even for methods that aren't overridden?

Thanks again for opening this issue!

[MiWeiss]

Thanks a lot for taking the time to write such a detailed answer and of course for maintaining this library! Much appreciated!

It seems like you're probably getting the No docstring message on methods that you're overriding.

Yes, this is correct.

I think that if a method is being overridden, an updated docstring is probably appropriate.

I see that in many cases you might be right. However, I don't agree in general. Let me mention two examples where I probably almost always want to inherit the superclass docstring:

• Methods that override an abc.abstractmethod method: In such a case, the interface (i.e., the documentation) is typically completely defined in the abstract class, a documentation update (and thus a change of contract) would in most cases defeat the purpose of the abstract method.

• Factory pattern: Users of my classes which are structured with a factory pattern are always coding against the superclass - thus, that's where the documentation is supposed to be. Subclasses are only assigned at runtime. Documentation on the methods of the implementing subclasses is thus pointless (as the class that will be used at runtime is unknown during development). Even worse, updating the docstring would probably indicate a change in the contract, which would break the factory pattern.

Sphinx and PyCharm also support docstring inheritance; thus, I do not seem to be the first one with this requirement ;-)

Workaround

Maybe the solution to the problem is a workaround?

Idea: It would be nice if I could, for example, add a #doc-check: inherited comment at the first line of a method, or an annotation @inherit_doc telling docstr_coverage that the method is documented. I acknowledge that the implementation of this workaround may be harder than it looks...

Ugly workaround: This works already and is compatible with Sphinx, but is quite ugly. Also, it's incompatible with PyCharm (and probably most other IDEs).

    def a_method(self):
        """doc inherited"""
        self.a_method.__doc__ = FooBar.a_method.__doc__

What's your view on that? Do you have any other proposals for workarounds?

[HunterMcGushion]

Those are great points! Thanks for outlining places where this would be necessary.

Your idea of using comments to mark overridden methods that shouldn't be checked (rather, their parent classes' methods should be checked) seems like a great solution! Another plus is the fact that it's a familiar pattern already in use by PyCharm's noinspection comments, Pytest's doctest options, Pylint, and others. As you say, my only concern is that the implementation may be harder than it looks haha. Are you at all interested in exploring this/making a PR?

In the meantime, a slightly less ugly workaround might be to use the --docstr-ignore-file option. Using the same sample_file.py above, we can ignore FooBaz.a_method by adding ignore_file.txt, with the following contents:

.*sample_file FooBaz.a_method

Then running docstr-coverage --skipclassdef --skipfiledoc --docstr-ignore-file=ignore_file.txt sample_file.py gives us 100% coverage. The problem with doing this is that the docstr-ignore-file ignores that method entirely, rather than deferring to its parent class. This issue is kind of mitigated by the fact that if the parent FooBar.a_method is missing its docstring, we would still get a warning for that. I suppose it works out as long as you ignore only the overridden methods of the subclasses, but it's something to keep in mind.

Another note about --docstr-ignore-file is that its pretty strict about paths, hence the .* before sample_file. Also, it works with Python module paths instead of filepaths, which is why it needs to be .*sample_file and *not `.sample_file.py`**.

I know this is certainly not ideal, but you've convinced me that this feature is necessary. If you are interested in making a PR, that would be fantastic and I'd love to help. Otherwise, I'm not sure when I'll be able to tackle this. Either way, thanks for suggesting such a useful addition!

[MiWeiss]

Sounds great.

I'll see if I find a couple of hours in the next days to see how this could be set up. However, my calendar is currently quite full and this looks like a major change; thus I don't want to promise a PR yet ;-)

One thing already worth mentioning: AST does not parse comments, thus I will have to add another dependency to docstr_coverage. Fine with you? EDIT: not so sure about this anymore, after all.

[HunterMcGushion]

I totally understand! Either way, thanks for your support and for bringing this up!

Yeah after some brief research, I was thinking we'd need a new dependency as well. I'd be fascinated to see a solution that doesn't add a new dependency if you found something, but I'm open to adding another one if necessary.

[MiWeiss]

The workaround is implemented and released and automatic detection of docstring inheritance is out of scope.

The issue can be closed