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

docstring code formatter: decide whether doctests should end with a empty PS2 prompt line or not #8908

Closed
BurntSushi opened this issue Nov 29, 2023 · 6 comments
Labels
docstring Related to docstring linting or formatting formatter Related to the formatter needs-decision Awaiting a decision from a maintainer

Comments

@BurntSushi
Copy link
Member

(This issue is forked off from discussion beginning at #7146 (comment).)

Currently, the way code snippet formatting in docstrings works is by collecting all of the lines in a single code snippet, feeding them into a recursive call to the formatter and then printing the resulting lines back into the docstring. For doctests in particular, the first line in a code snippet is always a PS1 prompt (starts with >>> ) and all subsequent lines (if any) are always PS2 prompts (starts with ... ). For example:

>>> with pl.Config(trim_decimal_zeros=False):
...     print(df)
...

This particular example contains a trailing empty line. This trailing empty line is collected as part of the code snippet and fed into the formatter. The formatter, as one might expect, trims this empty line. The end result is that the reformatted code snippet looks like this:

>>> with pl.Config(trim_decimal_zeros=False):
...     print(df)

The point of this issue is to decide whether this is desirable behavior or not. It came up in feedback from @stinodego after I reformatted the code snippets in polars. If one looks at the diff carefully, you can see some examples where there was an empty PS2 prompt line at the end of a code snippet (and got trimmed). But there are also plenty of code snippets that do not have an empty PS2 prompt line at the end of it. It therefore seems clear that we cannot unconditionally add an empty PS2 prompt line at the end of every reformatted doctest code snippet.

For example, it seems at least clear that a code snippet with only a PS1 prompt line should not have an empty line added after it. That is, we wouldn't want a reformatted code snippet to look like this:

>>> foo(x)
...

I say this because I've looked at a number of real doctests in the wild and I don't think I've ever seen one written like this.

But if the code snippet does contain at least one non-empty PS2 prompt line, then I think it's less clear whether we should add a trailing empty line or not. Namely, if you look at the diff to polars closely, not all code snippets with non-empty PS2 prompt lines contain a trailing empty line. That is, we can't really discern a consistent pattern from existing practice from this one example.

Another data point is a diff from applying code snippet reformatting to CPython. I can't discern a consistent rule there either. Some code snippets with non-empty PS2 prompt lines have a trailing empty line and others don't.


Speaking for me personally, I favor following the existing formatting rules and trimming the new line. But this might be something we want to collect user feedback on.

Pinging folks involved in the discussion in #7146: @stinodego @MichaReiser @ofek @pawamoy

@BurntSushi BurntSushi added docstring Related to docstring linting or formatting formatter Related to the formatter needs-decision Awaiting a decision from a maintainer labels Nov 29, 2023
@BurntSushi BurntSushi added this to the Formatter: Stable milestone Nov 29, 2023
@ofek
Copy link
Contributor

ofek commented Nov 29, 2023

I think trimming such new lines is the correct behavior 👍

@pawamoy
Copy link

pawamoy commented Nov 29, 2023

Agree with @ofek here, IMO it should trim the new line. Copying the snippet and pasting it in a REPL demands interaction anyway, so even if the user has to hit enter after paste, it's fine. The trailing PS2 empty line is just a consequence of how the REPL works, and shouldn't be replicated in snippets (even though these snippets legitimately try to mimic the input/output of the REPL). Depending on the rendering method/generator, and/or client side customization of the selection behavior, the prompts might even be left out of the selected content, so the trailing new line would make even less sense.

@stinodego
Copy link
Contributor

But if the code snippet does contain at least one non-empty PS2 prompt line, then I think it's less clear whether we should add a trailing empty line or not. Namely, if you look at the diff to polars closely, not all code snippets with non-empty PS2 prompt lines contain a trailing empty line. That is, we can't really discern a consistent pattern from existing practice from this one example.

I think there is actually a consistent way to determine when there should be a trailing .... It occurs in the Python console when an indentation decrease happens, i.e. after defining a function or using a context manager. But not after a simple statement like foo(x).

That being said, I'm completely fine with these being trimmed. Humans writing doc examples also naturally leave them out. They are currently enforced and added by blackdoc in our code base.

@keewis
Copy link

keewis commented Dec 7, 2023

adding a trailing ... after the end of block statements like if or with is a code-style choice, so this is very subjective (see keewis/blackdoc#52): it makes the code example a appear little less dense. Obviously, trailing empty lines should be removed for anything that is not a block statement.

Following the argument that doctest is basically a copy-pasted REPL cell would actually mean that you'd have to remove the trailing empty line after a block statement (which is what blacken-docs does, AFAIR). However, this appears to be changing in python=3.13 (unfortunately, I can't remember where I got that bit of information).

@MichaReiser
Copy link
Member

@BurntSushi from what I understand is that we currently strip the empty line. I'm fine with leaving it as is because we haven't received any feedback that clearly shows that it should be the other way round. The "worst" outcome is that we have to add an option controlling whether it should be stripped or not, which I think would be fine. So I would recommend closing this for now and re-visit when it comes up as an issue

@BurntSushi
Copy link
Member Author

SGTM!

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

No branches or pull requests

6 participants