Fix and refactors for docparams extension
#7398
Conversation
The ``re_only_desc`` regex did not match for white and characters after ``\n``, so some description-only lines weren't getting matched. In addition, lookaheads were added to ``re_param_line`` (i.e. make sure the type group is not followed by a new line (``\n``)). Lastly, named groups (ala Perl regular expressions) were added for slightly improved clarity. Co-authored-by: Hendry, Adam <adam.grant.hendry@gmail.com>
DanielNoord
added
Maintenance
Pull Request Test Coverage Report for Build 2989227539
💛 - Coveralls |
This comment has been minimized.
This comment has been minimized.
|
@Pierre-Sassoulas This allows the PR of @adam-grant-hendry to only focus on the |
There was a problem hiding this comment.
I like the new PR to separate the two modifications.
Are we sure the style of the docstring is detected properly ( Pandas seems to be using numpy, based on this), so it's strange that there is so much violations. On the other hand it seems they suggest to check with pylint so it's entirely possible that if pylint was not detecting it previously then some of their docstring are wrong...
Also shouldn't we add explicit functional tests for the new test cases ? Regexes are pretty easy to get wrong so regression tests would really help in case of future changes.
|
https://github.com/PyCQA/pylint/runs/8144779659?check_suite_focus=true If you search for "fh" in the logs it shows that we are not actually recognizing these parameters to begin with. With this change we are recognising them, but do not consider them correctly typed. Take: It's clear that they are trying to put types on the first line and not descriptions. But in the first example there are also descriptions and not only types. Should we raise |
I think we should not raise |
We are. I think we don't check signatures for this check. I'll look into accepting these docstrings in |
Let's keep raising it then, we can't know that the description is trying to define the typing in plain English.
I think this could be merged if we add some functional tests to formalize and check what we want to prevent future regression. |
Wouldn't it make more sense then to just accept anything that is being written on the line that is intended for typing? And not bother about determining whether it is actually typing. |
|
I thought it was supposed to be like |
I think that's Sphinx |
DanielNoord
added
the
Needs backport
|
@Pierre-Sassoulas Let's get final confirmation from the primer, but I think I fixed the issue here 😄 |
This comment has been minimized.
This comment has been minimized.
|
@Pierre-Sassoulas The last commit is debatable. Do we want to raise I think that if you're documenting the type in a docstring you'll probably add a description if you really want to. |
This comment has been minimized.
This comment has been minimized.
I'm personally rather lax with param documentation in docstring because if you use proper variable name then the param description is VERY often just the variable name with a more natural english capitalization... So if we have type information I would say it's voluntary that the param is not documented further... Now, I'm not using the docparam checker myself. Is my opinion the same as the one from persons that would actually use this checker... ? Probably not. |
Yeah I guess it depends on what "documentation" is. Is it any form of documentation or only a specific description? |
@DanielNoord @Pierre-Sassoulas I'd be careful: documentation is code. The end user may add an examples section, and if they use incorrect names, that should fail. In these days, that documentation may even also include |
Yeah agreed, but take: def func(arg):
"""
Parameters
----------
arg : int or None
""" Should we really raise |
|
Agree with the last point, I say let's not raise but maybe also let's wait for additional opinion, 2.16 is not out yet we can wait a little before merging. |
No, but I would raise |
Yeah, but that should be its own message probably. Something like |
Strong agree here, this is full project in itself. Let's raise one of the two existing messages and let the user sort it out. |
That's completely fair. I understand 👍 |
Pierre-Sassoulas
added
Backported
and removed
Needs backport
* Fix and refactors for ``docparams`` extension The ``re_only_desc`` regex did not match for white and characters after ``\n``, so some description-only lines weren't getting matched. In addition, lookaheads were added to ``re_param_line`` (i.e. make sure the type group is not followed by a new line (``\n``)). Lastly, named groups (ala Perl regular expressions) were added for slightly improved clarity. Co-authored-by: Hendry, Adam <adam.grant.hendry@gmail.com>
* Fix and refactors for ``docparams`` extension The ``re_only_desc`` regex did not match for white and characters after ``\n``, so some description-only lines weren't getting matched. In addition, lookaheads were added to ``re_param_line`` (i.e. make sure the type group is not followed by a new line (``\n``)). Lastly, named groups (ala Perl regular expressions) were added for slightly improved clarity. Co-authored-by: Hendry, Adam <adam.grant.hendry@gmail.com>
Type of Changes
Description
Some refactors based on the work in #7360
I'd like to see how the primer reacts to separate some of the large changes we were seeing in that PR.