Make black remove leading and trailing spaces from one-line docstrings: fixes #1738 #1812

[MarkCBell]

Resolves #1738 Black removes leading and trailing spaces in multiline docstrings but fails to remove them from one-line docstrings

[MarkCBell]

It appears that the primer check are checking how black affects a number of different projects. Since this changes how single line docstrings are processed a number of these now change.

[felix-hilden]

I've seen the Black team cautious about other than formatting changes in the source code elsewhere, but given that such processing is already done with other docstrings, this seems warranted. Also, the code is simple enough.

The test run logs seem to have timed out. I don't know if they can be rerun manually, or does the branch need to be updated, but having fresh results would be nice. Also there are conflicts with the current master, so fixing that would also update the branch and let you run the tests once more!

I'm not a maintainer though, so you'll sadly have to wait for someone else. Hopefully someone is able to do a quick review!

[ichard26]

I think the main issue is that it's not clear how we want to handle docstring formating still. IRCC the current behaviour was a mistake added by a somewhat unrelated feature addition, so we might just rollback to the previous formatting, IDK and I haven't looked into it.

[felix-hilden]

Oh, I see! Thanks for the information!

[MarkCBell]

Also there are conflicts with the current master, so fixing that would also update the branch and let you run the tests once more!

I have resolved the conflicts (and opened #2072 to deal with problems in the PR checking process).

As expected, the issues about the Primer checks into how black affects a number of different projects still remain.

[JelleZijlstra]

Looks good to me except for a suggested improvement in wording

[JelleZijlstra]

IRCC the current behaviour was a mistake added by a somewhat unrelated feature addition

In fact it was added on purpose in #1053.

[JelleZijlstra]

Also (sorry for the stream of consciousness), you'll have to update the black-primer configuration to reflect any additional projects with expected changes, assuming the changes all look right.

I checked out a few of them and did notice one special case:

 class DummyContext:
-    """ """
+    """"""

I don't really know why they're doing this, but it looks a lot weirder with no space in the middle. Maybe we should avoid completely emptying a docstring that consists only of spaces, instead replace it with a single space? I don't feel strongly about this; if others here prefer the completely empty string we can just keep things as is.

[ichard26]

Sorry for not submitting my review earlier, to be honest, I was extremely confused by the docstring handling Black has right now. I thought Black modified docstrings much more than what is actually true. I really don't care about the indention or space cleanup Black does, the only thing that isn't immediately clear to me is the behaviour for docstring quotes, in particular the behaviour described in #1635.

Putting this file through Black on master d960d5d:

def test():
    """This is one possibility.

    Very important stuff here.
    """


def test2():
    """This is another one possibility.

    Very important stuff here."""


def test3():
    """
    This is another one possibility.

    Very important stuff here.
    """


def test4():
    """
    What about this?
    """


def test5():
    """What about this?

    Some people also like to have an empty line at the end of the doc string.

    """


def test6():
    """Single line docstrings also exist."""


def test7():
    """
    Some people like them like this.
    """


def test8():
    """Or like this.
    """


def test9():
    """
    Or even like this."""


def test10():
    """Plus this.....

    style"""

returns this diff:

--- test.py	2021-04-11 22:07:57.113291 +0000
+++ test.py	2021-04-11 22:08:00.916690 +0000
@@ -42,12 +42,11 @@
     Some people like them like this.
     """
 
 
 def test8():
-    """Or like this.
-    """
+    """Or like this."""
 
 
 def test9():
     """
     Or even like this."""

which rubs me in the wrong way, either Black becomes more consistent with the "if can be collapsed into a single line docstring, it will" rule (i.e. collapse test6-10 not just test8), OR just remove this handling to let the user have full choice with where the quotes go.

P.S. perhaps we could document the handling (indention fixing, whitespace removal, etc.) Black has so it's easy to find and clear?

[ichard26]

Please resolve the merge conflict and other than this looks good to me (edit: also please resolve Jelle's comments!). Thanks!

Regarding """""" vs """ """, I'd go with """ """ too, but just like Jelle I don't feel strongly about this, so I'm open to some good ol' convincing by others :)

[ichard26]

PS. I'm pretty sure #1812 can also be closed by this PR. Can you link in the description so we don't forget to close it?