-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Add improved docstring processing #2885
base: main
Are you sure you want to change the base?
Conversation
diff-shades results comparing this PR (b8c169f) to main (836acad). The full diff is available in the logs under the "Generate HTML diff report" step.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for writing this up and putting it under the preview style.
I'm not yet convinced we should do this, since it could be disruptive. diff-shades currently gives us results only for pyanalyze (which happens to be my code), because it runs black with preview = True
. As you see, I currently use a different docstring style :)
I'd be curious to see the result on more open-source code. Could you push a change to enable the new style without preview mode, so that we can see the diff-shades result? (Obviously we should turn it back to preview-only before merging.)
This reverts commit 3895c4d.
Here’s diff-shades’ report when the new style is set as the default (rather than hiding it behind |
The most recent commit addresses an example of the single-line docstring line-length issue. It’s a bit complicated. Re-reading PEP 8, it seems the 72-character limit does not apply to docstring summary lines as they’re not ‘flowing long blocks of text with fewer structural restrictions’. I think this means the most sensible limit to apply would be the normal length limit (i.e. usually 88 characters). However, PEP 257 says that, for one-line docstrings, ‘the closing quotes are on the same line as the opening quotes.’ Combined, these directives result in a peculiar docstring-line-length limbo, in which a docstring such as """Test that the code option finds the pyproject.toml in the current directory.""" is prohibited. It must either violate line-length, violate single-line-docstring-quotes-on-same-line, or be shortened, or, oddly, lengthened, since adding body text would allow the quotes to move. I can think of three ways Black can handle this:
I don’t think the articles in the One advantage of the style of having quotes on their own line at the start of multi-line docstrings is that changing code to this style doesn’t cause another copy of this issue. (The other reasons I had for choosing this style were that it seemed more popular in the #144 thread (e.g. this poll), and that it’s consistent with the way Black places brackets over multiple lines.) |
Thanks for the PR! For the record, this is the style I like best too. But since we're not sure, we should weigh in the fact that this is yet another case for changing AST (#2150) and the various https://github.com/psf/black/labels/F%3A%20strings%20%2F%20docstrings issues. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few thoughts:
- This is going to make even more people upset for similar reasons as Latest
black
changes ASTs #2150. And I can't blame them; it's easy to imagine a change like this breaking some docstring processing system, and Black's AST safety guarantee is an important feature. Perhaps we should bite the bullet and add an option to disable docstring processing entirely. Such an option would disable both this PR and the existing docstring changes. New options aren't great, but an option that allows users to opt out of one part of Black's style is more defensible. - The one specific choice in this PR that I'm having trouble with is putting the opening quotes on their own line. It's clear that in Black itself previously we mostly made the opposite choice. So it may be less disruptive to put the first line right after the opening quotes.
@@ -10,9 +10,10 @@ | |||
square = Square(4) # type: Optional[Square] | |||
|
|||
def function(a:int=42): | |||
""" This docstring is already formatted |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you change the input section here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the same file works for the tests with --preview
and without. I wanted to limit the set of test files where that differed to the two main docstring ones (docstring_no_string_normalization.py
, docstring.py
and their _preview
counterparts). I did a similar thing to fmtonoff.py
and comments.py
.
I could try pushing a version with this quote style to see what diff-shades says. |
diff-shades with the same-line opening quotes style is +13 296 / ‒29 637 out of 2 147 073. Testing this led me to discover an issue with this quote-placement style: def foo():
"""Heading:
content that is supposed to be indented
""" gets converted to def foo():
"""Heading:
content that is supposed to be indented
""" by Black’s current docstring indentation logic. |
A summary of the points I can see in favour of each docstring start style: Summary line has opening quotes
Summary line after opening quotes
"""Heading for indented section
Indented section text.
""" and """Oops! The body is misindented.
Misindented body.
""" With the opening quotes on their own line, relative indentation can just be maintained. (See #1698.) |
It still says ‘1 change requested’ with a menacing red document symbol. I’m not sure if this means that I’m supposed to request a review again, or if there’s something that I’ve missed. I’ve gone ahead and requested one, so I apologise for the spam if it’s the latter. |
it means that .@JelleZijlstra requested changes and they have to either dismiss their review or make a new review. Github leaves it up until they take an action, nothing you can do. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code is solid (and thanks for persisting with this PR!) but as before I am not happy with the new lines inserted into multiline docstrings. The change is disruptive enough in Black itself that the PR becomes hard to read.
@JelleZijlstra as a person who's used both styles I really can't decide between them. Putting the quotes on their own line sometimes makes sense, but I feel as if the docstring can be easier to read when text follows the quotes on the same line. Ideally I'd like if we didn't solve this question and just made it togglable, but I understand that there'd rather not be a configuration option :^) |
Hmm… as often seems to be the case with Git, I managed to get myself confused. While trying to update this PR, I seem to have ended up overwriting your a583a32 – I’m sorry about that. Hopefully it’s either OK as is, or is at least reversible. I probably ought to be more careful around things with ‘force’ in the name! |
Description
One of the things discussed in issue #144 was quote placement. Reading through the discussion, there seemed (to me, at least) to be general support for normalising this.
This does two things. For very short docstrings,
becomes
and for docstrings of more than one line, a single newline at the start and (pre-indent) end are enforced, so
and
both become
which seemed to be the more popular style (compared with having no newline line at the start) in the #144 thread.
At the moment, any docstring with only one line of content will be converted to the one-line form. This may be problematic if it is very long, but I'm not sure what the line-length limit should be if this were changed: the normal line length limit, or 72 characters as specified by PEP 8 (which I'm not sure actually applies to single-line docstrings), or something else?
I changed some tests files where the docstring formatting didn’t seem relevant, so as not to have two copies of too many test files.
I didn’t reformat Black’s own source code with this change. There are parts of Black’s source that are already affected by
--preview
, so maybe this should change at the same time.Checklist - did you ...