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

Re-indent the contents of docstrings #1053

Merged
merged 4 commits into from
May 8, 2020
Merged

Conversation

alexmv
Copy link
Contributor

@alexmv alexmv commented Oct 10, 2019

This cleans up bancek@0889797 by @bancek based on the feedback there.

@zsol
Copy link
Collaborator

zsol commented Oct 13, 2019

Can you help me understand why this change is needed? I don't understand it either from the description of this PR, or the referenced commit, or the test cases (which by the way pass even on master). I'd rather not reverse-engineer the new code.

@bancek
Copy link
Contributor

bancek commented Oct 14, 2019

This change is needed to preserve the docstring indent when formatting codebases with 2 spaces indentation to 4 spaces.

Source file:

class MyClass:
  """Multiline
  class docstring
  """

  def method(self):
    """Multiline
    method docstring
    """
    pass

After running black:

class MyClass:
    """Multiline
  class docstring
  """

    def method(self):
        """Multiline
    method docstring
    """
        pass

Expected:

class MyClass:
    """Multiline
    class docstring
    """

    def method(self):
        """Multiline
        method docstring
        """
        pass

@zsol
Copy link
Collaborator

zsol commented Oct 14, 2019

Nice! @alexmv can we include that example in the test cases, please? And maybe another one that tests the conversion from overindented (more than 4 spaces) docstrings.

@bancek
Copy link
Contributor

bancek commented Oct 14, 2019

@alexmv can you split that into two commits so that my original commits stays as is (for contribution)?

@zsol
Copy link
Collaborator

zsol commented Oct 14, 2019

I'm going to squash it anyway when merging, though

@alexmv
Copy link
Contributor Author

alexmv commented Oct 15, 2019

Whoops -- sorry for not wording the commit more carefully to make its need clear. Tests all fail for me on master, now, and include the test case above and an additional deeper indent.

I've added an explicit Co-authored-by: trailer which should preserve your attribution more explicitly, @bancek.

@alexmv
Copy link
Contributor Author

alexmv commented Oct 18, 2019

Any other comments on this?

@ambv
Copy link
Collaborator

ambv commented Oct 20, 2019

Can you point me at an issue where this was agreed on?

@alexmv
Copy link
Contributor Author

alexmv commented Oct 22, 2019

I wasn't aware that a separate issue needed to be created for what seemed like a reasonable bugfix with code provided.

There seems to be reasonable agreement here on the PR. What sort of agreement are you looking for in a separate issue?

@jboning
Copy link

jboning commented Oct 22, 2019

In #144 (where @bancek originally shared his patch) there seems to be some consensus that adjusting docstrings is in scope for black, though the discussion there focuses more on leading/trailing wihtespace than on indentation.

@jboning
Copy link

jboning commented Oct 22, 2019

It might be good to have a testcase that includes lines in the docstring with extra leading whitespace, e.g.:

def foo():
    """
    Lorem ipsum dolor sit amet.

    Consectetur adipiscing elit:
     - sed do eiusmod tempor incididunt ut labore
     - dolore magna aliqua
       - enim ad minim veniam
       - quis nostrud exercitation ullamco laboris nisi
     - aliquip ex ea commodo consequat
    """
    pass

Alex Vandiver and others added 3 commits November 12, 2019 14:37
Keeping the contents of docstrings completely unchanged when
re-indenting (from 2-space intents to 4, for example) can cause
incorrect docstring indentation:

```
class MyClass:
  """Multiline
  class docstring
  """

  def method(self):
    """Multiline
    method docstring
    """
    pass
```
...becomes:
```
class MyClass:
    """Multiline
  class docstring
  """

    def method(self):
        """Multiline
    method docstring
    """
        pass
```

This uses the PEP 257 algorithm for determining docstring indentation,
and adjusts the contents of docstrings to match their new indentation
after `black` is applied.

A small normalization is necessary to `assert_equivalent` because the
trees are technically no longer precisely equivalent -- some constant
strings have changed.  When comparing two ASTs, whitespace after
newlines within constant strings is thus folded into a single space.

Co-authored-by: Luka Zakrajšek <[email protected]>
This reduces the cyclomatic complexity to a level that makes flake8 happy.
@alexmv
Copy link
Contributor Author

alexmv commented Nov 12, 2019

Thanks for that suggestion, @jboning -- I've added that test, along with one for a too-deep closing """, which this also resolves.

Rebased atop latest master.

@zsol zsol self-assigned this May 8, 2020
@ambv
Copy link
Collaborator

ambv commented May 8, 2020

Let's merge this. Don't forget to add it to changelog.

@zsol zsol merged commit a4c11a7 into psf:master May 8, 2020
zsol added a commit that referenced this pull request May 8, 2020
@zsol
Copy link
Collaborator

zsol commented May 8, 2020

Thanks for contributing @alexmv and @bancek !

@bancek
Copy link
Contributor

bancek commented May 8, 2020

@alexmv thanks for the PR!
@zsol thanks for accepting this into black.

@hugovk
Copy link
Contributor

hugovk commented May 22, 2020

Please see #1452.

alexmv added a commit to alexmv/black that referenced this pull request May 22, 2020
The PEP 257 algorithm used in psf#1053 results in trimming trailing
whitespace in docstrings -- see psf#1415 and fixes in psf#1417.  Removing
trailing whitespace may result in four quotes in a row:

   def foo():
       """"Some content
       and more "here" """
       pass

When closing the docstring, escape any trailing quote characters that
it matches, if they are not already escaped.

Fixes psf#1446.
alexmv added a commit to alexmv/black that referenced this pull request May 22, 2020
The PEP 257 algorithm used in psf#1053 results in trimming trailing
whitespace in docstrings -- see psf#1415 and fixes in psf#1417.  Removing
trailing whitespace may result in four quotes in a row:

    def foo():
        """"Some content
        and more "here" """
        pass

When closing the docstring, escape any trailing quote characters that
it matches, if they are not already escaped.

Fixes psf#1446.
alexmv added a commit to alexmv/black that referenced this pull request May 22, 2020
0;95;0c
The PEP 257 algorithm used in psf#1053 results in trimming trailing
whitespace in docstrings -- see psf#1415 and fixes in psf#1417.  Removing
trailing whitespace may result in four quotes in a row:

    def foo():
        """"Some content
        and more "here" """
        pass

When closing the docstring, escape any trailing quote characters that
it matches, if they are not already escaped.

Fixes psf#1452.
alexmv added a commit to alexmv/black that referenced this pull request May 22, 2020
The PEP 257 algorithm used in psf#1053 results in trimming trailing
whitespace in docstrings -- see psf#1415 and fixes in psf#1417.  Removing
trailing whitespace may result in four quotes in a row:

    def foo():
        """"Some content
        and more "here" """
        pass

When closing the docstring, escape any trailing quote characters that
it matches, if they are not already escaped.

Fixes psf#1452.
alexmv added a commit to alexmv/black that referenced this pull request Jul 31, 2020
The PEP 257 algorithm used in psf#1053 results in trimming trailing
whitespace in docstrings -- see psf#1415.

Adjust the algorithm to preserve leading and trailing whitespace; this
reverts the changes in psf#1417.  This diverges from PEP 257, but better
retains the contents of the docstring.

Fixes psf#1452 because we no longer can end up with four trailing quotes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants