Firstly, thanks for docsig, it's great!
One area where I still am finding 'mistakes' in my docstring signatures though is where I've added the description of an __init__
signature in the __init__
docstring itself, rather than the class docstring. If I miss a parameter, or document a parameter that doesn't exist there, there's no way to configure docsig to raise an error, without it also being unhappy about the signature being documented in the __init__
docstring rather than the class docstring.
Minimal example
I think this is understood that this isn't covered (it's on the README page), so putting this inside a 'details' as I think my job here is more persuading that it's worth docsig supporting this, rather than describing what it currently does - but I want to have that too of course.
For a minimal example, consider this code:
class SomeClass:
\"\"\"This class does something useful.\"\"\"
def __init__(self, first_arg: bool, second_arg: str):
\"\"\"Create a new instance of SomeClass
:param first_arg: this does exist
:param non_existent_arg: this arg doesn't actually exist!
\"\"\"
pass
This has incorrect documentation (so rendered docs will show incorrectly), but will pass a docsig
check with default configuration):
from docsig import docsig
string_to_check = """
class SomeClass:
\"\"\"This class does something useful.\"\"\"
def __init__(self, first_arg: bool, second_arg: str):
\"\"\"Create a new instance of SomeClass
:param first_arg: this does exist
:param non_existent_arg: this arg doesn't actually exist!
\"\"\"
pass
"""
docsig(string=string_to_check, check_class=True)
Note though that if I turn on check_class
, even a correctly documented __init__
will cause an error E103: parameters missing
:
from docsig import docsig
string_to_check = """
class SomeClass:
\"\"\"This class does something useful.\"\"\"
def __init__(self, first_arg: bool, second_arg: str):
\"\"\"Create a new instance of SomeClass
:param first_arg: this does exist
:param second_arg: this exists too and completes coverage
\"\"\"
pass
"""
docsig(string=string_to_check, check_class=True)
this gives:
5 in SomeClass
--------------
class SomeClass:
"""
:param None: ✖
:param None: ✖
"""
def __init__(✖first_arg, ✖second_arg)?:
E103: parameters missing
However, I think this is a valid way of writing docstrings, and one where docsig would add value if it checked this style of writing them (I found this by realising I had made a mistake of this kind in my project).
In fact, my reading of PEP257's multi-line docstring section is that this is the recommended way of writing __init__
signatures:
The class constructor should be documented in the docstring for its __init__ method
From this paragraph on docstrings for classes:
The docstring for a class should summarize its behavior and list the public methods and instance variables. If the class is intended to be subclassed, and has an additional interface for subclasses, this interface should be listed separately (in the docstring). The class constructor should be documented in the docstring for its __init__ method. Individual methods should be documented by their own docstring.
I think probably for backwards-compatibility, we would probably want to leave the check of an __init__
docstring only running when check_class
is True
.
Would you be interested in docsig supporting this?
If so, I may be able to work on a PR myself, if we can agree on an approach.
Some initial thoughts on supporting this
My impression is that it's [these lines](https://github.com/jshwi/docsig/blob/7fc0f02c71f8cb469c41763dee2ccf0bba2f6e38/docsig/_function.py#L292-L295) (copied below for inline clarity that set the docstring of `__init__` method's to the parent class's docstring, which prevents this being possible:
self._docstring = _Docstring(
node.doc_node if not self.isinit else self._parent.doc_node,
ignore_kwargs,
)
There's a couple of approaches that I wonder if they're worth considering:
- allow the constructor(/
__init__
) of _function._Docstring
to take a str
for node
, as well as _ast.Const
or None
, and then replace:
node.doc_node if not self.isinit else self._parent.doc_node,
with:
node.doc_node if not self.isinit else self._parent.doc_node.value + "\n" + node.doc_node.value,
This will fix the problems I currently have, but does allow weird things where some params are documented in the class docstring, and others in the __init__
docstring, which is undesirable, but seems unlikely to happen.
- Add a class method on
_function._Docstring
to create a _Docstring
from a string, which the above code will call in the self.isinit
case (this preserves the constructor signature of _Docstring
but is otherwise similar to the above
- Move the
if not self.isinit
logic inside _Docstring.__init__
, create _Matches(...)
for both the __init__
docstring and the parent class docstring, and use whichever one gets matches for the _Docstring._string
, _Docstring._args
and _Docstring._returns
. If neither, choose the class docstring, if both match maybe raise an error? Might have to be a new one. The downside of this is that it's a bigger change, and there's more compute involved in deciding which docstring to use for __init__
methods, which is then 'throwaway work' if check_class
isn't even true.