Adds support to ignored-argument-names in DocstringParameterChecker
#3842
Conversation
There was a problem hiding this comment.
@luigibertaco thanks a lot for this improvement.
My main concern is about the fact that pylint could accept to document an unused/ignored parameter.
pylint/extensions/docparams.py
Outdated
| self, "ignored-argument-names", set() | ||
| ) | ||
| if ignored_argument_names: | ||
| ignored_argument_names = { |
There was a problem hiding this comment.
It may be not a good idea to use the same name for two different types. ignored_argument_names is initially a re pattern object and then is affected to a set.
Maybe the set could be named expected_but_ignored_arg_names or something like that?
pylint/extensions/docparams.py
Outdated
| @@ -443,6 +444,16 @@ class constructor. | |||
| expected_argument_names.update(arg.name for arg in arguments_node.kwonlyargs) | |||
| not_needed_type_in_docstring = self.not_needed_param_in_docstring.copy() | |||
|
|
|||
| ignored_argument_names = get_global_option( | |||
| self, "ignored-argument-names", set() | |||
There was a problem hiding this comment.
If the option is retrieved then it will be a re pattern object, if it is not retrieved why not get None instead of set?
pylint/extensions/docparams.py
Outdated
| @@ -476,7 +487,7 @@ class constructor. | |||
| params_with_type, | |||
| "missing-type-doc", | |||
| not_needed_type_in_docstring, | |||
| expected_argument_names, | |||
| expected_argument_names - ignored_argument_names, | |||
There was a problem hiding this comment.
I'm wondering if we should also modify the expected_argument_names in the calls to _compare_different_args?
| :param _: Another argument. | ||
| :type _: float |
There was a problem hiding this comment.
Does it makes sense to document an unused/ignored parameter?
IMHO i don't think so. By modifying the calls to _compare_different_args then pylint should be able to detect this and will emit differing-param-doc. Maybe we can think about changing the message in this case to something around useless-param-doc...
There was a problem hiding this comment.
My main reason for this was to avoid breaking changes. But I agree with you, I haven't thought about adding a new message, that is a good idea.
luigibertaco
force-pushed
the
enhancement-3800-ignore-argument-names-on-docstring-parmaeters
branch
from
847b520
to
1ffeba1
Compare
…docstring-parmaeters
luigibertaco
force-pushed
the
enhancement-3800-ignore-argument-names-on-docstring-parmaeters
branch
from
1f867d2
to
2378dc1
Compare
There was a problem hiding this comment.
@luigibertaco thanks a lot for this great work!
I left only two suggestions and a question concerning your docstring.
After that we will be clear to merge!
pylint/extensions/docparams.py
Outdated
| :param set found_argument_names: argument names found in the | ||
| docstring |
There was a problem hiding this comment.
Maybe I'm wrong but it seems that your docstring is a mix between different ones. Usually for sphinx type docstring you put the argument type on a different line.
There was a problem hiding this comment.
I am with you on that, it was new for me but it is the style that is being used on the file as you can see on the existing methods: https://github.com/PyCQA/pylint/pull/3842/files/2378dc143d99e66febb1c3daeb66fbbb1b3e7d7c#diff-61228d78a7c97a0961324b4e7e6eb65eR383
I then chose to keep the same style instead of mixing them.
Happy to change for what you think would be more appropriate. What is your opinion on that?
There was a problem hiding this comment.
@luigibertaco it seems as this style is used only in the _compare_missing_args, _compare_ignored_args and _compare_different_args methods. I suppose it is a mistake. Do you mind correcting it?
There was a problem hiding this comment.
Hi @hippo91, I've fixed the sphinx style on the methods, but it got me thinking if pylint shouldn't have raised a warning for that.
pylint/extensions/docparams.py
Outdated
| "W9019": ( | ||
| '"%s" useless ignored parameter documentation', | ||
| "useless-param-doc", | ||
| "Please check ignored parameter names in declarations.", |
There was a problem hiding this comment.
Why not something more directive such as:
Please remove the ignored parameter documentation
```?
pylint/extensions/docparams.py
Outdated
| "W9020": ( | ||
| '"%s" useless ignored parameter type documentation', | ||
| "useless-type-doc", | ||
| "Please check ignored parameter names in type declarations.", |
Steps
doc/whatsnew/<current release.rst>.Description
Adds support to ignored arguments from the
ignored-argument-namesconfig such as "_", for instance.To avoid breaking changes it also accepts ignored argument names to be documented.
Type of Changes
Related Issue
Closes #3800