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

pylint.extensions.docparams default options are not working correctly #5223

Closed
adamcunnington-gdt opened this issue Oct 27, 2021 · 6 comments · Fixed by #5296
Closed

pylint.extensions.docparams default options are not working correctly #5223

adamcunnington-gdt opened this issue Oct 27, 2021 · 6 comments · Fixed by #5296
Labels
Bug 🪲 Documentation 📗 False Positive 🦟 A message is emitted but nothing is wrong with the code
Milestone

Comments

@adamcunnington-gdt
Copy link

adamcunnington-gdt commented Oct 27, 2021

Bug description

The documentation for pylint.extensions.docparams describes that by default, accept-no-yields-doc and accept-no-return-doc are yes (along with other configuration options).

However, if I run pylint myfile.py with no pylint configuration except for enabling the plugin*, I see lots of entries of:

... W9011: Missing return documentation (missing-return-doc)
... W9013: Missing yield documentation (missing-yield-doc)

* my only configuration is:

[tool.pylint.master]
load-plugins = ["pylint.extensions.docparams"]

In all situations, I am deliberately excluding :yields: and :rtype: from the docstring because I am using type annotations.

Am I missing something or is the documented behaviour wrong?

Pylint version

pylint 2.11.1
astroid 2.8.4
Python 3.9.5 (default, May  8 2021, 00:04:35) 
[GCC 8.3.0]

OS / Environment

Debian Buster

@adamcunnington-gdt adamcunnington-gdt added Bug 🪲 Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling labels Oct 27, 2021
@DanielNoord
Copy link
Collaborator

Would you be able to share the code that is creating these warnings?

For future reference.

Default values seem to be correct:
https://github.com/PyCQA/pylint/blob/e8713873813bfab5fafb04b0fb8b5221011faa41/pylint/extensions/docparams.py#L179-L198

Tests also seem fine on first inspection:
https://github.com/PyCQA/pylint/blob/e8713873813bfab5fafb04b0fb8b5221011faa41/tests/extensions/test_check_return_docs.py#L35-L58

@adamcunnington-gdt
Copy link
Author

adamcunnington-gdt commented Oct 27, 2021

@DanielNoord sure, it's happening for every single one of my functions/methods.

Here is an example:

def format_dataset(dataset_ref: bigquery.DatasetReference) -> str:
    """Return a BQ SQL-compatible <project>.<dataset> representation of the BQ dataset reference.

    :param dataset_ref: the BQ dataset reference to format
    """
    return f"{dataset_ref.project}.{dataset_ref.dataset_id}"
pylint -d C0301 bqtemplatemanager/
************* Module bqtemplatemanager.helpers
bqtemplatemanager/helpers.py:11:0: W9011: Missing return documentation (missing-return-doc)

@DanielNoord
Copy link
Collaborator

@Pierre-Sassoulas This behaviour seems by design.
These settings are checked with:
https://github.com/PyCQA/pylint/blob/e8713873813bfab5fafb04b0fb8b5221011faa41/pylint/extensions/docparams.py#L329-L331

is_valid is defined here:
https://github.com/PyCQA/pylint/blob/e8713873813bfab5fafb04b0fb8b5221011faa41/pylint/extensions/_check_docs_utils.py#L323-L330
or here:
https://github.com/PyCQA/pylint/blob/e8713873813bfab5fafb04b0fb8b5221011faa41/pylint/extensions/_check_docs_utils.py#L525-L532

Note that both work with or instead of and.

Thus, whenever some part of the docstring is correct we no longer check the accept-no-return-doc setting (same goes for no-yield) and can never return prematurely.
If this is expected behaviour we should probably document it.

I have tried removing is_valid() but then 11 tests fail which seems to indicate this is by design. The relevant commit that added this functionality is:
21424f2

@adamcunnington-gdt
Copy link
Author

adamcunnington-gdt commented Oct 28, 2021

Thanks for looking into it.

I think it is contradictory for this to be by design though because then these settings are completely meaningless. Note the wordings of the settings, e.g. "Whether to accept totally missing raises documentation in the docstring of a function that raises an exception." If the check only correctly happens when there is no docstring (or no correct bits within the docstring), then what's the point of the option?

If you are to update the documentation to explain the currently implemented behaviour, you are going to have to change the wording to be less about accepting specific scenarios and more about describing when to warn on an absent docstring fullstop.

@DanielNoord
Copy link
Collaborator

@Pierre-Sassoulas Pinging you again because I think you might have missed this.

If you just didn't have time to look at this, please consider this message unsent!

@Pierre-Sassoulas Pierre-Sassoulas added Documentation 📗 Good first issue Friendly and approachable by new contributors Help wanted 🙏 Outside help would be appreciated, good for new contributors Bug 🪲 False Positive 🦟 A message is emitted but nothing is wrong with the code and removed Bug 🪲 Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling Help wanted 🙏 Outside help would be appreciated, good for new contributors Good first issue Friendly and approachable by new contributors labels Oct 31, 2021
@Pierre-Sassoulas
Copy link
Member

Pierre-Sassoulas commented Oct 31, 2021

It look like the original intent in #914 was to permit to disable part of the check, so I guess it's a (somewhat solidified with tests since ?) bug and we should update the code, and tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug 🪲 Documentation 📗 False Positive 🦟 A message is emitted but nothing is wrong with the code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants