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

Docparams does not recognize class docstring when placed in __init__ #6692

Open
tcamise-gpsw opened this issue May 24, 2022 · 4 comments
Open
Labels
Enhancement ✨ Improvement to a component Good first issue Friendly and approachable by new contributors Hacktoberfest Help wanted 🙏 Outside help would be appreciated, good for new contributors Minor 💅 Polishing pylint is always nice Needs PR This issue is accepted, sufficiently specified and now needs an implementation

Comments

@tcamise-gpsw
Copy link

tcamise-gpsw commented May 24, 2022

Bug description

The documentation states that "Constructor parameters can be documented in either the class docstring or the init docstring, but not both.

However, I see that the docstring is only recognized when in the class docstring.

Consider this simple main.py:

"""MyModule"""

class MyClass:
    def __init__(self, variable: int) -> None:
        """_summary_

        Args:
            variable (int): _description_
        """
        self.my_variable = variable

    def method1(self) -> bool:
        """_summary_"""
        return bool(self.my_variable)

    def method2(self) -> int:
        """_summary_"""
        return 2 if self.my_variable else 1

This will raise a missing-class-docstring error.
If I move the docstring from __init to the Class, it passes.
Also of note, if I put the docstring in both places, I do not get multiple-constructor-doc error.

Configuration

[MAIN]
load-plugins=pylint.extensions.docparams

Command used

pylint main.py

Pylint output

************* Module main
main.py:3:0: C0115: Missing class docstring (missing-class-docstring)

------------------------------------------------------------------
Your code has been rated at 8.57/10 (previous run: 8.57/10, +0.00)

Expected behavior

I expect to not receive this error when the docstring is in __init__
Also, I expect to receive multiple-constructor-doc if I put the docstring in both places.

Pylint version

Python 3.9.10
I tested both pylint 2.14.0b and 2.13.9

OS / Environment

Windows Git Bash

Additional dependencies

No response

@tcamise-gpsw tcamise-gpsw added the Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling label May 24, 2022
@tcamise-gpsw tcamise-gpsw changed the title Docparams does not recognize class docstring when in __init__ Docparams does not recognize class docstring when placed in __init__ May 24, 2022
@Pierre-Sassoulas Pierre-Sassoulas added Enhancement ✨ Improvement to a component Proposal 📨 and removed Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling labels May 25, 2022
@Pierre-Sassoulas
Copy link
Member

Pierre-Sassoulas commented May 25, 2022

Regarding the missing-class-docstring I don't think it's a false positive, you can document the constructor parameter in the constructor itself and still want a class docstring (you shouldn't document the constructor parameter in the class docstring though.).

So:

"""MyModule"""

class MyClass:
   """_summary_

   Args:
       variable (int): _description_
   """
   def __init__(self, variable: int) -> None:
       """_summary_

       Args:
           variable (int): _description_
       """
       self.my_variable = variable

   def method1(self) -> bool:
       """_summary_"""
       return bool(self.my_variable)

   def method2(self) -> int:
       """_summary_"""
       return 2 if self.my_variable else 1

should raise something because variable is documented twice.

@tcamise-gpsw
Copy link
Author

So this means that if I want to put the constructor docstring in __init__, I am also required to put at least some docstring (i.e. a summary of the class) in the class docstring. I guess that is correct based on PEP 257:

The docstring for a class should summarize its behavior and list the public methods and instance variables. If the class is intended to be subclassed, and has an additional interface for subclasses, this interface should be listed separately (in the docstring). The class constructor should be documented in the docstring for its init method. Individual methods should be documented by their own docstring.

In case anyone else is trying to accomplish something similar...ultimately I am trying to enforce the presence of class docstrings and also validate their content (using darglint). However, darglint can only validate constructors when they are in __init__. So that, coupled with this (now I realize is expected) pylint behavior makes this currently impossible. I'll try to follow up on the relevant darglint ticket..

Anyway thanks for looking into this. I personally don't care about the missing multiple-constructor-doc error but if you want to keep this open and implement it, more power to you 👍

@Pierre-Sassoulas Pierre-Sassoulas added Help wanted 🙏 Outside help would be appreciated, good for new contributors Minor 💅 Polishing pylint is always nice Good first issue Friendly and approachable by new contributors and removed Proposal 📨 labels May 25, 2022
@Pierre-Sassoulas Pierre-Sassoulas added the Needs PR This issue is accepted, sufficiently specified and now needs an implementation label Jun 24, 2022
@clavedeluna
Copy link
Contributor

What's the consensus on implementation? Is fixing the issue as stated originally the way to go?

@Pierre-Sassoulas
Copy link
Member

The issue as originally stated is not a false positive, what should be implemented now is the check talked about in the last comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement ✨ Improvement to a component Good first issue Friendly and approachable by new contributors Hacktoberfest Help wanted 🙏 Outside help would be appreciated, good for new contributors Minor 💅 Polishing pylint is always nice Needs PR This issue is accepted, sufficiently specified and now needs an implementation
Projects
None yet
Development

No branches or pull requests

3 participants