fix(numpy-param-doc): allow "default" in params #7360
Conversation
|
@DanielNoord @Pierre-Sassoulas Would you please kindly help review? Thank you. |
Pierre-Sassoulas
added
the
False Negative 🦋
This comment has been minimized.
This comment has been minimized.
|
@adam-grant-hendry The primer tests shows that we would be emitting a lot more messages which I don't really expect. Perhaps we can take a test from there? |
One difficulty I have is that |
This comment has been minimized.
This comment has been minimized.
Yes, I added a fragment with my last push. |
Fix was for issue pylint-dev#6211. The PR that fixes this is pylint-dev#7360. See pylint-dev#6211
Fix was for issue pylint-dev#6211. The PR that fixes this is pylint-dev#7360. See pylint-dev#6211
adam-grant-hendry
force-pushed
the
fix/numpy-param-doc
branch
from
a470251
to
b241a92
Compare
Just did, but couldn't figure out how to do that without force pushing. I used
Yes, the regex is not working, but I don't know why. |
This comment has been minimized.
This comment has been minimized.
adam-grant-hendry
force-pushed
the
fix/numpy-param-doc
branch
from
d4eef42
to
3aaba09
Compare
|
@DanielNoord @Pierre-Sassoulas This should be working now. I wrote a test script to help me out: test_script.pyfrom __future__ import annotations
import re
import typing
from pylint.extensions._check_docs_utils import NumpyDocstring
if typing.TYPE_CHECKING:
from pylint.extensions._check_docs_utils import Docstring
def space_indentation(s: str) -> int:
"""The number of leading spaces in a string.
:param str s: input string
:rtype: int
:return: number of leading spaces
"""
return len(s) - len(s.lstrip(" "))
def _is_section_header(line: str) -> bool:
return bool(re.match(r"\s*-+$", line))
def min_section_indent(section_match: re.Match[str]) -> int:
return len(section_match.group(1))
def _parse_section(
section_re: re.Pattern[str],
doc: str,
) -> list[str]:
section_match = section_re.search(doc)
if section_match is None:
return []
min_indentation = min_section_indent(section_match)
entries: list[str] = []
entry: list[str] = []
is_first = True
for line in section_match.group(2).splitlines():
if not line.strip():
continue
indentation = space_indentation(line)
if indentation < min_indentation:
break
# The first line after the header defines the minimum
# indentation.
if is_first:
min_indentation = indentation
is_first = False
if indentation == min_indentation:
if _is_section_header(line):
break
# Lines with minimum indentation must contain the beginning
# of a new parameter documentation.
if entry:
entries.append("\n".join(entry))
entry = []
entry.append(line)
if entry:
entries.append("\n".join(entry))
return entries
def match_param_docs(
checker: NumpyDocstring,
doc: str,
) -> tuple[set[str], set[str]]:
"""Matches parameter documentation section to parameter documentation rules."""
params_with_doc = set()
params_with_type = set()
entries = _parse_section(checker.re_param_section, doc)
entries.extend(_parse_section(checker.re_keyword_param_section, doc))
for entry in entries:
match = checker.re_param_line.match(entry)
if not match:
continue
print(match)
print()
# check if parameter has description only
re_only_desc = re.match(r"\s*(\*{0,2}\w+)\s*:?\n\s*\w*$", entry)
print(f're_only_desc: {re_only_desc is not None}')
if re_only_desc:
param_name = match.group('param_name')
param_desc = match.group('param_type')
param_type = None
else:
param_name = match.group('param_name')
param_type = match.group('param_type')
param_desc = match.group('param_desc')
print(f"Param Name: '{param_name}'")
print(f"Type: '{param_type}'")
print(f"Description: '{param_desc}'")
if param_type:
params_with_type.add(param_name)
if param_desc:
params_with_doc.add(param_name)
return params_with_doc, params_with_type
if __name__ == '__main__':
docstrings = [
"""This is a regression test for issue `#6211`_.
False negative of "missing param doc" was being issued when "default" used in
NumPy-style docs. This test should return no errors.
Parameters
----------
number : int, default 0
The number parameter
.. _`#6211`:
https://github.com/PyCQA/pylint/issues/6211
""",
"""No errors raised when pipe symbol used for or.
`PEP 604`_ allows writing Union types as X | Y. Can be enabled in Python <3.10
using `from __future__ import annotations`.
Parameters
----------
arg1
The first arg
arg2 : str or None, default=None
The second arg
.. _`PEP 604`:
https://peps.python.org/pep-0604/
""",
"""Reported in https://github.com/PyCQA/pylint/issues/4035.
Parameters
----------
arg : int
The number of times to print it.
option : {"y", "n"}
Do I do it?
option2 : {"y", None, "n"}
Do I do it?
""",
"""function foobar ...
Parameters
----------
arg1 : int
arg2 : int
arg3 : str
""",
"""function foobar ...
Parameters
----------
arg1:
description
arg2: str
""",
"""
Helper to assert that left and right cannot be added.
Parameters
----------
left : object
right : object
msg : str, default "cannot add"
""",
"""
Assert that comparison operations with mismatched types behave correctly.
Parameters
----------
left : np.ndarray, ExtensionArray, Index, or Series
right : object
box : {pd.DataFrame, pd.Series, pd.Index, pd.array, tm.to_array}
""",
"""Generate many datasets.
Parameters
----------
data : fixture implementing `data`
Returns
-------
Callable[[int], Generator]:
A callable that takes a `count` argument and
returns a generator yielding `count` datasets.
""",
"""
Read HTML file from formats data directory.
Parameters
----------
datapath : pytest fixture
The datapath fixture injected into a test by pytest.
name : str
The name of the HTML file without the suffix.
Returns
-------
str : contents of HTML file.
""",
"""
Check that operator opname works as advertised on frame
Parameters
----------
opname : str
Name of the operator to test on frame
alternative : function
Function that opname is tested against; i.e. "frame.opname()" should
equal "alternative(frame)".
frame : DataFrame
The object that the tests are executed on
has_skipna : bool, default True
Whether the method "opname" has the kwarg "skip_na"
check_dtype : bool, default True
Whether the dtypes of the result of "frame.opname()" and
"alternative(frame)" should be checked.
check_dates : bool, default false
Whether opname should be tested on a Datetime Series
rtol : float, default 1e-5
Relative tolerance.
atol : float, default 1e-8
Absolute tolerance.
skipna_alternative : function, default None
NaN-safe version of alternative
"""
]
np_checker = NumpyDocstring(None)
for ndx, docstr in enumerate(docstrings):
print(f'========= Docstring {ndx+1} =========')
params_with_description_only, params_with_a_type = match_param_docs(np_checker, docstr)
print()
print(f'Parameters with a description only: {params_with_description_only}')
print(f'Parameters with a type: {params_with_a_type}')
print()A couple notes:
re_param_section = re.compile(
_re_section_template.format(r"(?:Args|Arguments|Parameters)"),
re.X | re.S | re.M,
)
There may be other literal/default types to add in the future, but for now I accounted just for ints because that solves the issue at hand: re_default_value = r"""((['"]\w+\s*['"])|(\d+)|(True)|(False)|(None))""" |
Pull Request Test Coverage Report for Build 2989408850
💛 - Coveralls |
This comment has been minimized.
This comment has been minimized.
adam-grant-hendry
force-pushed
the
fix/numpy-param-doc
branch
from
45f0035
to
32f71f7
Compare
@DanielNoord @Pierre-Sassoulas After using my script, I found the original code was matching colon ( have the form This is incorrect per the NumPy Style Guide. Descriptions must go on the next line indented. Only types can occur after the colon on the same line. is in fact missing documentation for the listed parameters. has incorrect types. The above distinction is quite relevant considering the prevelance of static type checkers for python nowadays. If |
|
@adam-grant-hendry That does indeed seem like we were incorrectly matching previously. Would you be willing to open a separate PR to fix that and add some regressions tests against that? |
This comment has been minimized.
This comment has been minimized.
I would, but I'm not sure I have time at this stage. I doubt I would be able to finish this PR even. The amount of fixes required is more than I originally envisioned, I'm sorry. At this stage, I'm simply getting by by:
|
|
@DanielNoord @Pierre-Sassoulas Two more drive-by comments:
e.g. This gives a def func(self, a: int, b: bool) -> None:
"""Does something funky.
This is where you explain the funk.
..warning:: ``a`` Explodes!
Don't use ``a``
Parameters
----------
a : int
An integer
b : bool
A bool
"""
def funkify(self, a: int | bool | str) -> str:
"""Get with the funk.
What the funk?!
Parameters
----------
a : int, bool, or str
This is an ``int``, ``bool``, or ``str``. How funky!
Returns
-------
b : str
Because why not return a ``str``
""" |
NumPy doc style guide permits using `default` in parameter signatures. Fixes: pylint-dev#6211
Change `x` to `number` in test method `regression_6211`. This was causing an `invalid-name` error.
Adds a news fragment for the issue.
Fix was for issue pylint-dev#6211. The PR that fixes this is pylint-dev#7360. See pylint-dev#6211
* Add Marc Byrne to the maintainer team
Adds a news fragment for the issue.
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.
The hyperlink in the body of test `regression_6211` was causing
problems, so this was moved into a reStructuredText directive.
Finally, the added
`from __future__ import annotations`
moved all lines in
`tests/functional/ext/docparams/docparams.py`
down one line, so the line numbers in `docparams.txt` were off by 1,
causing tests to fail. This was corrected.
Also remove lookahead from `re_param_line` as tests pass without it.
DanielNoord
force-pushed
the
fix/numpy-param-doc
branch
from
32f71f7
to
8f89ec5
Compare
Both of these should probably be their own issue, right? I have updated this PR after the merge of #7398. Let's see what the primer thinks! |
This comment has been minimized.
This comment has been minimized.
|
@Pierre-Sassoulas We have a similar issue as discussed previously here. Should we just accept anything that comes on the |
There was a problem hiding this comment.
Sorry for the delay I did not realize this was waiting for my input. I think we should accept anything from the type field, because being able to tell valid type from text is hard, and doing something like "None or MyClass" is frequent enough that the primers message was truncated because there's so much new message. (Removing from the 3.0 milestone because this is the kind of thing that can be merged later)
towncrier create <IssueNumber>.<type>which will beincluded in the changelog.
<type>can be one of: new_check, removed_check, extension,false_positive, false_negative, bugfix, other, internal. If necessary you can write
details or offer examples on how the new change is supposed to work.
and preferred name in
script/.contributors_aliases.jsonType of Changes
Description
NumPy doc style guide permits using
defaultin parameter signatures.Closes ##6211