black icon indicating copy to clipboard operation
black copied to clipboard

Blank line after docstring... or not.

Open PabloLec opened this issue 4 years ago • 15 comments

Describe the style change

Currently, black does not check blank lines between functions docstring and body. That means both these options are valid:

def foo():
    """docstring"""
    
   return 0
def foo():
    """docstring"""
   return 0

As black is uncompromising and opinionated, I guess this should be taken into account.

Personally I tend to put a blank line but looking at the only potential reference PEP 257, there is no blank lines in given examples.

Whether we enforce the blank line or not, black should take care of this as this results in easy style inconsistency.

I'd say we should follow PEP 257 no blank line example. Comment your opinion so that this issue can be refered to in a PR fixing this.

If there is no consensus, we could organize a jousting match to settle this. :goberserk:

PabloLec avatar Jul 12 '21 10:07 PabloLec

Isn't this a case of (referring the docs):

Black will allow single empty lines inside functions, and single and double empty lines on module level left by the original editors, except when they’re within parenthesized expressions.

So working as intended and documented.

we could organize a jousting match to settle this

Reasonable proposal, I like this!

hukkin avatar Jul 12 '21 10:07 hukkin

Well, indeed the doc describes this behavior, but I think this should be treated separately. As on module level, black does not allow beginning blank lines and enforce a final blank line, this same logic should apply at function level.

e.g. this code looks inconsistent and should be reformatted:

def foo():
    """docstring"""
   return 0

def bar():
    """docstring"""

   return 0

def baz():
    """docstring"""
   return 0

PabloLec avatar Jul 12 '21 11:07 PabloLec

I agree and would also like to see Black have an opinion in the case where there is no docstring, my personal preference is no line between function def and first line of the body, but having it take a position one way or the other matters more to me than which position.

echoing @PabloLec this code looks inconsistent and should be reformatted.

def foo():
   return 0

def bar():

   return 0

def baz():
   return 0

tolomea avatar Aug 16 '21 11:08 tolomea

Also: https://github.com/psf/black/issues/1872

stinos avatar Oct 04 '21 09:10 stinos

Also related, perhaps not worth a separate issue (please mention if it should be): the placement of the last triple quotes for last line of the docstring itself.

Sample; all of these are left as-is, but most of these should in my opinion get a treatment. This is probably my number one issue with black. Please stick with being opinionated :)

class Foo1:
    """Multi.

    Line."""

    pass


class Foo2:
    """Multi.

    Line.
    """

    pass


def Func1():
    """Single."""
    pass


def Func2():
    """Single."""

    pass


def Func3():
    """Multi.

    Line."""
    pass


def Func4():
    """Multi.

    Line."""

    pass


def Func5():
    """Multi.

    Line.
    """

    pass


def Func6():
    """Multi.

    Line.
    """
    pass

On one hand doing the same as for single-line docstrings, triple quotes on same line and no newline after, it is consistent.

On the other hand the example for https://www.python.org/dev/peps/pep-0257 shows the triple quotes on the next line and no blank line after, which seems to be used most in the standard library (only had a quick look though).

Since this is about opinions, let's express one: I'd be in favor of consistency so no blank line after (at least for functions, for classes and modules should remain as-is) and triple quotes attached to the text (after all that's how the docstring starts as well).

stinos avatar Oct 14 '21 14:10 stinos

I'm not 100 % sure if we should do it. I'd say I'm moderately in favor of it 😄 But if we do, my preference would be no blank lines:

def f():
    pass

def f():
    """Doc."""
    return stuff

def f():
    """
    Multiline doc.

    With some explanation.
    """
    return stuff

That might be influenced a lot by the fact that an IDE will highlight the docstring very differently from the following code, which is a good visual aid. But most people should be using one anyway.

felix-hilden avatar Oct 14 '21 14:10 felix-hilden

def f():
    """
    Multiline doc.

    With some explanation.
    """
    return stuff

Now that you mention it; I didn't consider the start of doc strings, but that's another case that should imo be treated and the newline erradicated; I'd like

def f():
    """Multiline doc.

    With some explanation."""
    return stuff

or perhaps the seemingly more common

def f():
    """Multiline doc.

    With some explanation.
    """
    return stuff

but not both, and also none of the other shenanigans like extra newlines before the opening """ or after the opening """. We have files like that lying around with some of these mixed, and it looks really off. And one uses black to fix that, normally. I get that docstrings might allow for some creativity, but at least fixing the beginning and end seems worth it.

stinos avatar Oct 14 '21 15:10 stinos

Well that's a can of worms too 😅 but I definitely prefer either on one line, or all at the same level, like I wrote above.

felix-hilden avatar Oct 14 '21 15:10 felix-hilden

I looked around a bit and this at the end of strings.fix_docstring (adjust to taste :))

    # Always attach text to the opening triple quotes.
    while len(trimmed) > 1 and not trimmed[0].strip():
        del trimmed[0]
    # Need to get rid of the indentation again if empty lines were deleted.
    trimmed[0] = trimmed[0].lstrip()
    # Always place trailing triple quotes on separate line if multi-line.
    if len(trimmed) > 1 and trimmed[-1].strip():
        trimmed.append(prefix)
    # Get rid of empty lines before trailing triple quotes.
    while len(trimmed) > 1 and not trimmed[-2]:
        del trimmed[-2]

seems to fix first/last lines of docstrings, combined with

        if (
            self.previous_line
            and current_line.is_triple_quoted_string
        ):
            return 0, 0

at the end of lines.EmptyLineTracker._maybe_empty_lines to get rid of lines before/after. That last one is almost certainly not sufiicient for things like docstrings in the middle of a class, but just to give an idea.

stinos avatar Oct 14 '21 16:10 stinos

@tolomea Your suggestion (without the docstrings) is being tracked in #902.

felix-hilden avatar Mar 01 '22 16:03 felix-hilden

Just a quick bump, I tried reaching out and offer help on main communication means a few times within the past months but no response so, yeah. I guess there are more important matters and blocking issues to consider first but this one seems close to black main purpose to me. Again, whatever the formatting will be, we need one.

PabloLec avatar May 20 '22 16:05 PabloLec

Module docstrings are already being handled by #2996. It's not a big stretch to start applying that elsewhere as well. We'll just have to agree to do it. And us (potentially) removing the lines in module docstrings would be in line with my preference above.

felix-hilden avatar May 20 '22 20:05 felix-hilden

I vote for no blank line after the docstring. I discovered this issue, because I was rewriting a file so that it complied with pydocstyle's standards. No matter which docstring style you use (PEP 257, numpydoc or Google), according to pydocstyle you should never have a blank line after a docstring. (source: http://www.pydocstyle.org/en/stable/error_codes.html: "No blank lines [...] after function docstring" is error code D202 and it's included in all three style conventions). I had to manually delete the trailing line for each function and would have loved it if black had taken care of it already.

mcnoat avatar Mar 28 '23 15:03 mcnoat

I know some tools will enforce PEP 257, but for non-tiny functions, it just looks weird to have no blank line, e.g.:

@torch.no_grad()
def inference(
    model: CompressionModel,
    x: torch.Tensor,
    skip_compress: bool = False,
    skip_decompress: bool = False,
    criterion: Optional[TCriterion] = None,
    min_div: int = 64,
    *,
    lmbda_idx: int = None,
    lmbda: float = None,
) -> dict[str, Any]:
    """Run compression model on image batch."""
    if lmbda_idx is not None:
        assert lmbda is None
        lmbda = model.lambdas[lmbda_idx]

    ...

versus:

@torch.no_grad()
def inference(
    model: CompressionModel,
    x: torch.Tensor,
    skip_compress: bool = False,
    skip_decompress: bool = False,
    criterion: Optional[TCriterion] = None,
    min_div: int = 64,
    *,
    lmbda_idx: int = None,
    lmbda: float = None,
) -> dict[str, Any]:
    """Run compression model on image batch."""

    if lmbda_idx is not None:
        assert lmbda is None
        lmbda = model.lambdas[lmbda_idx]

    ...

YodaEmbedding avatar Jun 11 '23 02:06 YodaEmbedding

As mentioned in https://github.com/psf/black/issues/4043, I lean towards not doing anything here.

I feel that if we do something here, it should not be to always strip blank lines. Doing so would run in to the same readability issues that has made previous blank line changes controversial (see the comment above, #4043, etc). It would be inconsistent with how Black treats module and class docstrings. The examples in this issue arguing for stripping the blank line are all toy code.

hauntsaninja avatar Nov 13 '23 08:11 hauntsaninja