Skip to content
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

Escaped '*' in docstring leads to missing-param-doc #5815

Closed
kasium opened this issue Feb 17, 2022 · 9 comments · Fixed by #6212
Closed

Escaped '*' in docstring leads to missing-param-doc #5815

kasium opened this issue Feb 17, 2022 · 9 comments · Fixed by #6212
Assignees
Labels
Discussion 🤔 Enhancement ✨ Improvement to a component
Milestone

Comments

@kasium
Copy link
Contributor

kasium commented Feb 17, 2022

Bug description

If a docstring contains the arguments for *args and **kwargs and RST is used, * must be escaped.

def myfunc(value: int, *args: Any) -> None:
    """This is myfunc.

    Args:
        \\*args: this is args
        value: this is value
    """
    print(*args, value)

However, the pylint docparams check doesn't like the escape

Configuration

No response

Command used

pylint --load-plugins pylint.extensions.docparams foo.py --enable all

Pylint output

************* Module foo
foo.py:4:0: W9015: "*args" missing in parameter documentation (missing-param-doc)

Expected behavior

Pylint should ignore escaping

Pylint version

pylint 2.12.2
astroid 2.9.3
Python 3.7.1 (default, Oct 22 2018, 13:16:18) [GCC]

OS / Environment

Linux

Additional dependencies

No response

@kasium kasium added the Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling label Feb 17, 2022
@kasium
Copy link
Contributor Author

kasium commented Feb 17, 2022

My suggesting: Before comparing the arguments, "clean" them somehow (e.g. remove escaping)

@DanielNoord
Copy link
Collaborator

We currently handle \*args and *args after discussion in #5406.
Change was introduced in https://github.com/PyCQA/pylint/pull/5464/files.

Should be fairly trivial to fix this. However, I wonder why this is needed. Do you need to escape the backslash twice? And are there instances where you need to escape it three or four times? Should we just allow an unlimited amount of backslash in front of the *?

@DanielNoord DanielNoord added Discussion 🤔 Enhancement ✨ Improvement to a component and removed Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling labels Feb 17, 2022
@kasium
Copy link
Contributor Author

kasium commented Feb 17, 2022

@DanielNoord If I use a single backslash I need to use "r-strings", else I get:

foo.py:8:8: W1401: Anomalous backslash in string: '\*'. String constant might be missing an r prefix. (anomalous-backslash-in-string)

Therefore you need to use \\ to avoid this error. I don't think that more than two backslashes are needed

@DanielNoord
Copy link
Collaborator

Is using r-string a problem in your case? This comment seems to indicate that that is the "correct" way to solve this:
#5406 (comment)

@kasium
Copy link
Contributor Author

kasium commented Feb 17, 2022

@DanielNoord I see your point. However, adding r-strings can lead to some issues in corner cases. I don't remember a concrete example but I guess I found one in the past. So, would you be open to accept a PR or do you think everybody should go with r-strings

@DanielNoord
Copy link
Collaborator

Hm, I'm not sure actually. I don't think it hurts to allow \\ but on the other hand: this is the exact case that r-strings are intended for. I can imagine some people might actually be interested in a consider-using-r-string checker in the CodeStyle checker.
If we can't find an actual breaking corner case I think disallowing this and thereby nudging users to the (arguably) intended way to do this could be a nice side-effect. So I'm -0.1 on this. I don't really oppose it, but personally think this is a nice side-effect (as long as nothing breaks of course).

@Pierre-Sassoulas
Copy link
Member

I agree with @DanielNoord here.

r-strings can lead to some issues in corner cases

I think we need to know what issues it's causing in what corner case to be able to make a decision. Personally I don't know of any :) Let's keep the discussion open, but as it's hard to prove a negative I'll put this in the 2.14 milestone so we actually close or fix at some point.

@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.14.0 milestone Feb 17, 2022
@kasium
Copy link
Contributor Author

kasium commented Feb 18, 2022

Thanks a lot. So I just added used your fix but it doesn't work. Please note, that I use Google-style. Does the fix only consider sphinx-style docstrings?

@DanielNoord DanielNoord self-assigned this Feb 18, 2022
@DanielNoord
Copy link
Collaborator

Hmm, I would need to investigate this. That could actually be the case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion 🤔 Enhancement ✨ Improvement to a component
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants