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

Black removes newline from a multiline docstring. #1635

Closed
omry opened this issue Aug 26, 2020 · 13 comments
Closed

Black removes newline from a multiline docstring. #1635

omry opened this issue Aug 26, 2020 · 13 comments
Labels
T: bug Something isn't working

Comments

@omry
Copy link

omry commented Aug 26, 2020

Describe the bug A clear and concise description of what the bug is.

class Foo:
    """foo
    """

Is being reformatted to:

class Foo:
    """foo"""

This is removing a newline.
arguably this is okay because it's a comment, but such comments are (as far as I know) accessible through object inspection.

Expected behavior A clear and concise description of what you expected to happen.
I am expecting black to not remove newlines from strings.

Environment (please complete the following information):

  • Version: black==20.8b1
  • OS and Python version: Linux/Python 3.8.5

Does this bug also happen on master?
Yes.

@omry omry added the T: bug Something isn't working label Aug 26, 2020
@cooperlees
Copy link
Collaborator

cooperlees commented Aug 26, 2020

Hi Omry, thanks for reporting.

Do you explicitly only want this only for docstrings? If so, black respects ‘\’.

I.e.

class Foo:
  “””Hello \
  “””

This would not be reformatted.

Black also respects multi lines variables with 20.8b1

foo = “””
Cooper made a string
“””

This is also not reformatted on my tests.

What you’re seeing is expected behavior with the new release I believe.

@omry
Copy link
Author

omry commented Aug 26, 2020

Hi @cooperlees,
This caught me off guard, specifically for my use case it's indeed a comment and don't mind the re-formatting.
I did notice that this works as I expect for variables.

the workaround does not seem to be generating the same string:

s1 = """hello
"""

s2 = """hello\ 
"""
print("s1", repr(s1))
print("s2", repr(s2))
$ python 1.py 
s1 'hello\n'
s2 'hello\\ \n'

@cooperlees
Copy link
Collaborator

cooperlees commented Aug 26, 2020

As usual, you've missed the point of my reply. Docstrings need the '\' to not join on the \n, like your original post only showed. Normal multi-lines strings should not be doing what you claim.

I am unable to repro:

If I

echo 's1 = """hello
"""

s2 = """hello\ 
"""
print("s1", repr(s1))
print("s2", repr(s2))
' > /tmp/omry.py
cooper-mbp1:bandersnatch cooper$ /tmp/black/bin/black --diff /tmp/omry.py
All done! ✨ 🍰 ✨
1 file would be left unchanged.

Neither multiline string here are reformatted for me ... Please provide a non docstring repro with 20.8b1 or master please that joins a non docstring mutli-line string and we can try fix it.

@omry omry closed this as completed Aug 26, 2020
@omry
Copy link
Author

omry commented Aug 26, 2020

The problem is specific to docstrings.
I used variables in the repro because it was easier for me to demonstrate that the proposed workaround changes the string like that.
Here is a repro that uses docstrings:

Code:
http://paste.ubuntu.com/p/w6mN4Y2R93/

class Foo:
    """hello
    """

class Bar:
    """hello\
    """

print("Foo" ,repr(Foo.__doc__))
print("Bar", repr(Bar.__doc__))

Output:

$ python 1.py 
Foo 'hello\n    '
Bar 'hello    '
$ black 1.py 
reformatted 1.py
All done! ✨ 🍰 ✨
1 file reformatted.

Trying to repro I possibly hit another bug:
if there is a trailing whitespace after the backslash it causes an internal error.
$ cat 1.py | pastebinit
http://paste.ubuntu.com/p/fW5KjwZrR6/
$ black 1.py
error: cannot format 1.py: INTERNAL ERROR: Black produced invalid code: EOF while scanning triple-quoted string literal (, line 10). Please report a bug on https://github.com/psf/black/issues. This invalid output might be helpful: /tmp/blk_prxe8kni.log
Oh no! 💥 💔 💥
1 file failed to reformat.

@omry omry reopened this Aug 26, 2020
@ambv
Copy link
Collaborator

ambv commented Aug 26, 2020

  1. The docstring whitespace change is deliberate.
  2. The invalid code produced when there's trailing whitespace after a backslash is indeed a bug in Black we need to fix.

@omry
Copy link
Author

omry commented Aug 26, 2020

Thanks ambv.
I personally prefer the formatted output, but I can imagine some cases where people would care about about the newlines in the docstring.

I think # fmt: off and # fmt: on are sufficient as a workaround in these cases.

@laloch
Copy link

laloch commented Aug 28, 2020

Could we please have a switch to disable the docstring reformatting altogether? Removing the leading whitespace may break docstrings containing tabular text. For instance this PLY/Yacc grammar rule:

    def p_flow_stmt(self, p):
        """flow_stmt : break_stmt
                     | continue_stmt
                     | return_stmt
                     | raise_stmt
                     | yield_stmt
        """
        p[0] = p[1]

ends up like:

    def p_flow_stmt(self, p):
        """flow_stmt : break_stmt
        | continue_stmt
        | return_stmt
        | raise_stmt
        | yield_stmt
        """
        p[0] = p[1]

@omry omry changed the title Black removes newline from a multiline string. Black removes newline from a multiline docstring string. Aug 28, 2020
@omry omry changed the title Black removes newline from a multiline docstring string. Black removes newline from a multiline docstring. Aug 28, 2020
@ambv
Copy link
Collaborator

ambv commented Aug 29, 2020

@laloch: I cannot reproduce this. Were the grammar rules using tab characters instead of spaces?

Nevermind, I was on the wrong branch. Yeah, we'll deal with this for the next release.

@ambv
Copy link
Collaborator

ambv commented Aug 29, 2020

@laloch In the mean time, a workaround for Black's behavior is starting your docstring text on a newline:

    def p_flow_stmt(self, p):
        """
        flow_stmt : break_stmt
                  | continue_stmt
                  | return_stmt
                  | raise_stmt
                  | yield_stmt
        """
        p[0] = p[1]

@laloch
Copy link

laloch commented Aug 29, 2020

Thanks @ambv! We have already resorted to pinning v19.10b0 in our CI setup for now.

@DrGFreeman
Copy link

DrGFreeman commented Sep 21, 2020

I also noticed that the "corrected" docstring will violate the line-length rule if the the docstring is within three characters of the line-length value. Reproduceable example with line-length = 88:

def f():
    """89 123456789 123456789 123456789 123456789 123456789 123456789 123456789 12345678
    """
    pass

will get changed to

def f():
    """89 123456789 123456789 123456789 123456789 123456789 123456789 123456789 12345678"""
    pass

where the docstring now exceeds 88 characters.

@TheFriendlyCoder
Copy link

I've been prototyping the use of Black for several of my work and personal projects, and hit this problem immediately while testing against my very first project. The fundamental problem as was mentioned in the last comment here, where the line length limit is not respected when wrapping quotes, which is a very common pattern for docstrings, makes the tool completely unusable. As such I would have hoped that this problem would have been considered a high-priority fix and yet there doesn't seem to be any action on it for months. This is definitely a deterrent for new users like myself who are considering your tool.

Also, just a quick note on the "workaround" that was suggested in some previous comments, where developers can escape the new-line characters in these cases using a "" character is not a practical solution imo. The purpose of Black is to "...give you speed, determinism, and freedom from pycodestyle nagging about formatting. You will save time and mental energy for more important matters." Saying that developers now how to understand nuanced handling of docstrings in Black goes against the stated purpose of the tool.

@JelleZijlstra
Copy link
Collaborator

The code mentioned somewhere above (http://paste.ubuntu.com/p/fW5KjwZrR6/) no longer crashes with current main. Other complaints in this bug are more general and don't seem actionable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T: bug Something isn't working
Projects
None yet
Development

No branches or pull requests

7 participants