psf / black

The uncompromising Python code formatter
https://black.readthedocs.io/en/stable/
MIT License
38.92k stars 2.46k forks source link

Blank line after docstring... or not. #2370

Closed PabloLec closed 6 months ago

PabloLec commented 3 years ago

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:

hukkin commented 3 years ago

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!

PabloLec commented 3 years ago

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
tolomea commented 3 years ago

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
stinos commented 3 years ago

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

stinos commented 3 years ago

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).

felix-hilden commented 3 years ago

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.

stinos commented 3 years ago
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.

felix-hilden commented 3 years ago

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.

stinos commented 3 years ago

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.

felix-hilden commented 2 years ago

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

PabloLec commented 2 years ago

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.

felix-hilden commented 2 years ago

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.

mcnoat commented 1 year ago

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.

YodaEmbedding commented 1 year ago

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]

    ...
hauntsaninja commented 11 months ago

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.

LLyaudet commented 6 months ago

Since Black has the goal of not allowing too much customization to avoid too many variations out in the nature, I think the original suggestion to not allow an empty line after function doc string is better:

And inside function body:

# Part 1 : Blabla
# Blabla
# Blabla
bla = blabla(x,y,z)
...
# Part 2 : Blabla
# Blabla
# Blabla
bla = blabla(x,y,z)
...

etc.

Moreover in linked issue https://github.com/psf/black/issues/902, someone argued about the case of a comment just after the docstring: https://github.com/psf/black/issues/902#issuecomment-1425285264 I think his example is bad, since if I remember correctly in PEP8, the comment should be inside the if block and not outside. But still, there may be a case where "part 1" starts with a function call just after docstring. And that case with a corresponding example would be relevant to the comment of @jenstroeger. I would not change my opinion for that and either accept that:

"""
My function:
- part 1: blabla
- part 2: blabla
- part 3: blabla
"""
# Part 1 : Blabla
# Blabla
# Blabla
bla = blabla(x,y,z)

is slightly ugly. Or that:

"""
My function:
- part 1: blabla
- part 2: blabla
- part 3: blabla
"""
#
# Part 1 : Blabla
# Blabla
# Blabla
bla = blabla(x,y,z)

is also slightly ugly and maybe more readable, or

"""
My function:
- part 1: blabla
- part 2: blabla
- part 3: blabla
"""
# ----------------------
# Part 1 : Blabla
# Blabla
# Blabla
# ----------------------
bla = blabla(x,y,z)

is ok. I think the trick of the empty line switched to an empty comment line, or a comment line with dashes is enough to have a nice visual and stop brainfucking about corner cases.

JelleZijlstra commented 6 months ago

I agree with @hauntsaninja above; we'll leave this unchanged.