-
Notifications
You must be signed in to change notification settings - Fork 49
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
Do more with parameter inspection #296
Conversation
Codecov Report
@@ Coverage Diff @@
## master #296 +/- ##
==========================================
+ Coverage 84.39% 84.58% +0.19%
==========================================
Files 23 23
Lines 3735 3782 +47
Branches 766 783 +17
==========================================
+ Hits 3152 3199 +47
Misses 386 386
Partials 197 197
Continue to review full report at Codecov.
|
These could be typos or outdated documentation. In either case, it's worth alerting the user to this, so the documentation can be fixed.
For the reader of the API docs, knowing the type of a parameter is a lot better than knowing nothing at all.
Instead of explicitly setting "Undocumented" as the body in specific cases, we just leave the body attribute as None and on presentation we substitute "Undocumented" if necessary. This removes an inconsistency where 'param' fields did have the "Undocumented" string while 'return' fields did not.
Most functions with None as their return type don't actually return a value. In cases where they do, the user can use a 'returns' or 'rtype' field to force the "Returns" entry to appear.
The information we extract from type annotations can be useful even if there is no docstring.
def handle_keyword(self, field: Field) -> None: | ||
name = self._handle_param_name(field) | ||
if name is not None: | ||
# TODO: How should this be matched to the type annotation? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two functions in Twisted that use @keyword
, but neither of them have type annotations at the moment, so I think it's fine to resolve this TODO later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to support @keyword
... can't we just use @param
? :)
I see that in the previous code keyword was the same as param... so maybe deprecated keyword
and just use @param
everywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The annotation would be useful only if all keywords have the same type: if for example one keyword is a str
and another keyword is an int
, the annotation will be object
or Any
or Union[int, str]
, which isn't very helpful to the end user. So the author will have to use type
fields to document the types of the individual keywords.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to support
@keyword
... can't we just use@param
? :)I see that in the previous code keyword was the same as param... so maybe deprecated
keyword
and just use@param
everywhere.
The purpose of @keyword
is to document keywords that can be captured by the **kwargs
parameter. I think it's useful to have a separate field for those, since otherwise we wouldn't be able to warn about @param
being used with a non-existing parameter name. That warning led to several documentation fixes in Twisted.
a0cd674
to
dc6ca1d
Compare
We already had this warning for the 'param' field, but documenting a type for a parameter that does not exist is equally worthy of receiving a warning. This also solves an issue where a 'type' field for a non-existing parameter that precedes the 'param' field hides the warning for that 'param' field, since having an entry in 'self.types' is how we decide that a parameter exists.
This is a partial implementation of twisted#229: ideally we'd want to warn on duplicate `type` and `keyword` fields as well, but for `type` this is currently not easy to implement and `keyword` isn't used enough to make it a priority.
dc6ca1d
to
b9d4e05
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good. Only minor comments.
It would have been nice to have the sample API doc enabled so that we could see in HTML in a browser all the cased for there is a new test
For now, I have used https://pydoctor--296.org.readthedocs.build/en/296/api/.
def handle_keyword(self, field: Field) -> None: | ||
name = self._handle_param_name(field) | ||
if name is not None: | ||
# TODO: How should this be matched to the type annotation? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to support @keyword
... can't we just use @param
? :)
I see that in the previous code keyword was the same as param... so maybe deprecated keyword
and just use @param
everywhere.
pydoctor/test/test_epydoc2stan.py
Outdated
def test_func_keyword(capsys: CapSys) -> None: | ||
"""Test handling of @keyword.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def test_func_keyword(capsys: CapSys) -> None: | |
"""Test handling of @keyword.""" | |
def test_func_parameter_as_keyword(capsys: CapSys) -> None: | |
"""A warning is raised when a parameter is documented as a @keyword.""" |
pydoctor/test/test_epydoc2stan.py
Outdated
) | ||
|
||
def test_func_arg_not_inherited(capsys: CapSys) -> None: | ||
"""Do not warn about non-existing parameters from inherited docstrings.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"""Do not warn about non-existing parameters from inherited docstrings.""" | |
"""No warning is emitted when a subclass is implementing an inherited function with different parameters.""" |
@@ -128,6 +159,83 @@ def f(a): | |||
classic_fmt = docstring2html(classic_mod.contents['f']) | |||
assert annotation_fmt == classic_fmt | |||
|
|||
def test_func_param_duplicate(capsys: CapSys) -> None: | |||
"""Warn when the same parameter is documented more than once.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we already have a test when the same parameter has multiple this is #229@type
definitions?
pydoctor/test/test_epydoc2stan.py
Outdated
|
||
def test_func_undocumented_return_something() -> None: | ||
"""When the returned value is undocumented and its type is not None, | ||
include a the "Returns" entry in the output. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this docstring....
So if the return type is not None and is not documented, it is added as an undocumented span.
I guess that it handles this case https://pydoctor--296.org.readthedocs.build/en/296/api/pydoctor.astbuilder.ASTBuilder.html#parseFile
Can we also include <tr class="fieldStart"><td class="fieldName">Returns</td><td colspan="2">
in the expected output?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is the case that it handles. I'll add a bit more detail to the docstring, in the hope that it will be easier to understand.
I added both <td>
tags in the expected output, that indeed makes the test easier to read. However, I left out the <tr>
, since that is not really part of what is being tested here and adding more output increases the chance that the test will require maintenance on unrelated changes.
Fixes #217
Known issues: (to be fixed after release)
type
orkeyword
fields yet:type
is not easy to implement,keyword
is barely used