[zsol]

Fixes #258

Note that this only changes behavior when black writes back to the file. In other cases like when using --diff or writing to stdout, I think it's reasonable to expect the caller to handle line endings.

[ambv]

I'll wait with landing this until I fix the tests on Travis.

[zsol]

In the meanwhile I'll figure out how to install precommit on windows and fix everything I've broken

[ambv]

You don't have to open it again, that's slow. You already have src_contents that you can check. Suggestion:

windows_newlines = src_contents.split("\n", 1)[0][-1:] == "\r"
newline = "\r\n" if windows_newlines else "\n"

[zsol]

src_contents uses universal newlines so "\r" is never in it :(

[zsol]

I'm not sure if opening a file for reading one more time right after reading through it is a huge performance hit, but if it turns out to be the case, we can reproduce the 10 or so lines of code in tokenize.open() to make sure we only do it once.

[ambv]

Right. OK, leave as is but open it in "rb" so it doesn't need to decode anything. The first line must be readable anyway since if it's not UTF8 (or ASCII on Python 2) it needs the # -*- coding: X -*- comment.

And as a total nit: instead of b"\r\n" in readline you can do readline[-2:] == b"\r\n" which is marginally faster still.

[zsol]

mypy complains about this line like so:

Cannot infer type argument 1 of "assertIn" of "TestCase"

I think it's confused because both arguments are bytes. Seems like a bug in mypy to me

[JelleZijlstra]

I think the typeshed stub is wrong. It has:

    def assertIn(self, first: _T, second: Iterable[_T],
                 msg: Any = ...) -> None: ...
    def assertNotIn(self, first: _T, second: Iterable[_T],
                    msg: Any = ...) -> None: ...

This should actually be Container, not Iterable.

[JelleZijlstra]

python/typeshed#2186 should fix it.

[coveralls]

Pull Request Test Coverage Report for Build 476

• 38 of 42 (90.48%) changed or added relevant lines in 2 files are covered.
• 76 unchanged lines in 1 file lost coverage.
• Overall coverage increased (+0.1%) to 96.272%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
black.py	22	24	91.67%
tests/test_black.py	16	18	88.89%

Files with Coverage Reduction	New Missed Lines	%
black.py	76	95.25%

Totals
Change from base Build 463:	0.1%
Covered Lines:	2453
Relevant Lines:	2548

---

💛 - Coveralls

[jgirardet]

@zsol

Note that this only changes behavior when black writes back to the file. In other cases like when using --diff or writing to stdout, I think it's reasonable to expect the caller to handle line endings.

I think It makes things very "compromising" and I thought it wasn't the purpose of black.
As the readme proposes to use black - to integrate black on editors, it gives some different behaviour depending the platform.

I think it would be more consistant that black preserves line ending with black - and black --diff.

Anyway If you disagree It should be mentioned in the readme

[ambv]

@zsol, I think @jgirardet is right. Especially on piping stdin to stdout we should be preserving existing newline style. But also on --diff, not taking existing format into account will lead to bugs in intergrations (like sublack).

I don't understand @jgirardet's comment about compromise. I just think the current behavior is still effectively buggy.

[jgirardet]

Just a joke, but that was probably bad English speaking 😊

[zsol]

fair enough, let me iterate on this

[jgirardet]

Hi guys,
I'm not sure anymore my sublack bug was this black bug. I tried it on windows python idle :

>> p = subprocess.Popen(
                ['black', '-'],
                stdin=subprocess.PIPE,
                stdout=subprocess.PIPE,
                stderr=subprocess.PIPE,)
>>> p.communicate(b"hgello\n")
(b'hgello\r\n', b'- already well formatted, good job.\r\n')

>>> p = subprocess.Popen(
                ['black', '-'],
                stdin=subprocess.PIPE,
                stdout=subprocess.PIPE,
                stderr=subprocess.PIPE,
		universal_newlines=True)
>>> p.communicate('hello\n')
('hello\n', '- already well formatted, good job.\n

Maybe this is just the popen standard behaviour ?

[ambv]

@jgirardet, wow, this is strange. I don't work with Windows so I don't really understand what the expected behavior should be with regards to stdin/stdout. Maybe what @zsol is already doing is valid after all on Windows?

[zsol]

@jgirardet what you're seeing here is just the effect of universal_newlines being on or off IMO.

>>> subprocess.Popen(['python', '-c', r'import sys; sys.stdout.write("\r\n")'], stdout=subprocess.PIPE).communicate()
(b'\r\n', None)
>>> subprocess.Popen(['python', '-c', r'import sys; sys.stdout.write("\r\n")'], stdout=subprocess.PIPE, universal_newlines=True).communicate()                                                                                              
('\n', None)

There is a bug in black in that it doesn't preserve newline styles as you can see in your first communicate() call. Without universal_newlines you get back the bytestream black outputs without any decoding or newline conversion. I bet if you do p.communicate(b'hello\r\n') you'll get the exact same output as p.communicate(b'hello\n'). After I fix this issue, they will be different.

[zsol]

Alright this is ready for review @ambv. I had to rewrite most of the code because we can't afford the luxury of consuming stdin twice

[ambv]

Thanks! ✨ 🍰 ✨

[jgirardet]

thanks @zsol.
This solved my problem in sublack for stdout.
I still have the issue with stderr:

All done! \u2728 \U0001f370 \u2728"<0x0d>"
1 file left unchanged."<0x0d>"

I added the quotes to show you the "<0x0d>" added at end of line.
I juste remove it with
error_message = err.decode(encoding).replace("\r\n", "\n").replace("\r", "\n")

[ambv]

@jgirardet, stderr is user-facing messages. It should have \r\n on Windows, this is how people are going to see them in the console.

[jgirardet]

ok
thats clear thanks

[blkpanther]

If I understand this correctly, black should preserve line endings when used with - (stdin). But I think 18.6b4 (the version that is currently supplied by pip) won't. I input code over stdin with \r\n line endings, I get output with \n. [am I wrong or missing something?]

[zsol]

@blkpanther Can you share how you're calling black and how you're piping code to it? Also, what environment (OS, locale) are you running this in?

In my case, black behaves as advertised (your understanding is correct):

> black --version
black, version 18.6b4
> echo -en 'hello\r\n' | black - | hd
All done! ✨ 🍰 ✨
1 file left unchanged.
00000000  68 65 6c 6c 6f 0d 0a                              |hello..|
00000007
> echo -en 'print (hello)\r\n' | black - | hd
reformatted -
All done! ✨ 🍰 ✨
1 file reformatted.
00000000  70 72 69 6e 74 28 68 65  6c 6c 6f 29 0d 0a        |print(hello)..|
0000000e

And my environment:

> uname -a
Linux Zeus 4.4.0-17134-Microsoft #137-Microsoft Thu Jun 14 18:46:00 PST 2018 x86_64 GNU/Linux
k> locale
LANG=en_US.UTF-8
LANGUAGE=
LC_CTYPE="en_US.UTF-8"
LC_NUMERIC="en_US.UTF-8"
LC_TIME="en_US.UTF-8"
LC_COLLATE="en_US.UTF-8"
LC_MONETARY="en_US.UTF-8"
LC_MESSAGES="en_US.UTF-8"
LC_PAPER="en_US.UTF-8"
LC_NAME="en_US.UTF-8"
LC_ADDRESS="en_US.UTF-8"
LC_TELEPHONE="en_US.UTF-8"
LC_MEASUREMENT="en_US.UTF-8"
LC_IDENTIFICATION="en_US.UTF-8"
LC_ALL=
``

[ambv]

@zsol, can you try it directly from PowerShell on Windows (without WSL)?

[zsol]

I have no idea how to verify this in PowerShell (or cmd.exe for that matter). I do know our test passes, but that doesn't use -

[zsol]

OK I wrote a new test for standardin and it fails. Investigating

edit: nevermind this was just an artifact of the test runner. Black behaves correctly:

import subprocess

out = subprocess.check_output(
    ["black", "-"], universal_newlines=False, input=b"print('hello')\r\n"
)
print(repr(out))

produces
b'print("hello")\r\n'

[blkpanther]

Works correct for me, too (sorry I caused a fuss).
Things to remember: mixed line ending input or no line ending input are corner cases. Black does treat them correctly but you might get confused.