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

BUG: Rule D413 does not catch missing single blank line or multiple blank lines at end of docstring #13284

Open
user27182 opened this issue Sep 8, 2024 · 4 comments
Labels
breaking Breaking API change docstring Related to docstring linting or formatting needs-decision Awaiting a decision from a maintainer

Comments

@user27182
Copy link

There is a bug with rule D413. With this rule, a single blank line is expected at the end of multiline docstrings.

But, this does not work in a few cases. E.g.:

Calling ruff check foo.py --select D413 --fix on this code does not insert a blank line at the end:

def foo():
    """Bar.

    Multiline docstring.
    """

In another case, we see that this rule works for some section names such as Returns, e.g. here a new line is correctly inserted at the end:

def foo():
    """Bar.

    Returns
    -------
    Nothing.
    """

But, it does not work for any general section heading. E.g. no blank line is inserted here after the Section section (but one is expected):

def foo():
    """Bar.

    Section
    -------
    Nothing.
    """

For the first case, I suppose there may be an argument that technically the docstring has no sections, and therefore the rule isn't being violated. And for the last case there may be an argument that only standard section names are supported (e.g. Parameters, Returns, See Also, etc.). But, in my view, I think that with this rule enabled, there should always be a blank line expected (and inserted with --fix) at the end of the docstring.

I have not tested this against the pydocstyle behavior though, not sure what the expected result is from that. Maybe a new rule could be added instead in case the current behavior is considered to be "correct"?

Also, related to #9451, with D413 we should expect only a single line at the end of a docstring. But this code passes (the two blank lines should be replaced with a single blank line):

def foo():
    """Bar.

    Multiline docstring.


    """

Same thing with this code, only one line is expected:

def foo():
    """Bar.

    Multiline docstring.

    Returns
    -------
    Nothing.

    
    """
@AlexWaygood AlexWaygood added the docstring Related to docstring linting or formatting label Sep 9, 2024
@MichaReiser
Copy link
Member

For the first case, I suppose there may be an argument that technically the docstring has no sections, and therefore the rule isn't being violated. And for the last case there may be an argument that only standard section names are supported (e.g. Parameters, Returns, See Also, etc.). But, in my view, I think that with this rule enabled, there should always be a blank line expected (and inserted with --fix) at the end of the docstring.

I think extending the rule to work after any, even unsupported section does make sense.

I don't think it's the right call to extend the rule to also flag docstrings without a section. It would change the rule's intention in a very significant way:

Checks for missing blank lines after the last section of a multiline docstring.

@MichaReiser MichaReiser added needs-decision Awaiting a decision from a maintainer breaking Breaking API change labels Sep 10, 2024
@MichaReiser
Copy link
Member

It's also worth noting that custom section names aren't recognized by other rules as well. For example, D214 doesn't report a section named Section but is raised for Parameters (playground)

@user27182
Copy link
Author

I don't think it's the right call to extend the rule to also flag docstrings without a section. It would change the rule's intention in a very significant way:

Checks for missing blank lines after the last section of a multiline docstring.

Fair enough. I guess this comes down to semantics/definitions. Is a section only considered to be a section if it has a heading (e.g. Parameters)? Or is the initial summary also considered a section, but without a heading?

Aside from technical definitions of what a section is, always having a single blank line makes the docstrings visually consistent, regardless of the presence of a heading or not. (In some cases, this is also useful to avoid RST syntax errors where a blank line may be required, though preventing syntax errors is likely out of scope for this rule).

So if it's decided that a multiline docstring without any sections is excluded from D413, then I propose having a separate rule that would cover this case. The rule I am looking for is perhaps not:

Checks for missing blank lines after the last section of a multiline docstring.

but instead

Checks that multiline docstrings have a single blank line at the end.

And by a single blank line at the end, I mean something like this, where there a line of pure whitespace at the end:

def foo():
    """Bar.

    Multiline docstring.

    """

I mention this because technically, according to PEP 257 the third line here with the triple quotes is considered a blank line:

def foo():
    """
    This is the second line of the docstring.
    """

And so based on the PEP 257 definition, ruff isn't applying D413 correctly, since the following example already has a blank line at the end and therefore isn't missing one (but ruff disagrees, and will add a second line here).

def foo():
    """Bar.

    Returns
    -------
    Nothing.
    """

So perhaps "section" and "blank line" need to be more clearly defined and documented to ensure users better understand what to expect from this rule.

@jhrr
Copy link

jhrr commented Sep 17, 2024

I'm also having this problem. The main issue for me is just to be able to ensure that there's an actual newline between the final closing """ no matter what (if it's a multiline docstring), just to enforce complete visual consistency everywhere.

So if it's decided that a multiline docstring without any sections is excluded from D413, then I propose having a separate rule that would cover this case.

As @user27182 says, all semantics aside, this would be my preferred solution also in order to satisfy that demand if this rule is not suitable for doing that job itself.

Thanks for all the effort on the library.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Breaking API change docstring Related to docstring linting or formatting needs-decision Awaiting a decision from a maintainer
Projects
None yet
Development

No branches or pull requests

4 participants