akaihola / darker

Apply black reformatting to Python files only in regions changed since a given commit. For a practical usage example, see the blog post at https://dev.to/akaihola/improving-python-code-incrementally-3f7a
https://pypi.org/project/darker/
Other
638 stars 55 forks source link

Avoid double newlines with `--stdout` #605

Closed akaihola closed 1 month ago

akaihola commented 3 months ago

Fixes #604

akaihola commented 3 months ago

@shane-kearns I added a unit test which tries to reproduce your original:

Running darker on the file normally (bin/darker --isort new-file.py) does not create bad line endings. The problem can be reproduced without VSCode thusly, stdout gets double line endings:

cat new-file.py | bin/darker --stdout --isort --stdin-file new-file.py - | py -3.11 -c "import sys; print(sys.stdin.buffer.read())"
b'import collections\r\n\r\nimport sys\r\n\r\n'

On Linux, both your original and the unit test version work correctly:

$ python -c 'from pathlib import Path; Path("new-file.py").write_bytes(b"import sys\r\nimport collections\r\n")'
$ cat new-file.py \
| darker --stdout --isort --stdin-file new-file.py - \
| python -c "import sys; print(sys.stdin.buffer.read())"
b'import collections\r\nimport sys\r\n'
$ pytest -v -k test_stdout_newlines
==== test session starts ====
platform linux -- Python 3.12.4, pytest-8.3.1, pluggy-1.5.0 -- /home/akaihola/.virtualenvs/darker/bin/python
cachedir: .pytest_cache
rootdir: /home/akaihola/prg/darker
configfile: pytest.ini
plugins: kwparametrize-0.0.3, darkgraylib-2.0.0
collected 640 items / 638 deselected / 2 selected                                                                                                                             

src/darker/tests/test_main.py::test_stdout_newlines[unix] PASSED
src/darker/tests/test_main.py::test_stdout_newlines[windows] PASSED

The unit test also seems to pass in CI on Windows.

Could you try out the unit test on your machine? Any other ideas for how to reproduce on CI?

shane-kearns commented 3 months ago

Your unit test passes here, but an equivalent unit test using subprocess + pipe to communicate fails as described in the issue:

@pytest.mark.parametrize("newline", ["\n", "\r\n"], ids=["unix", "windows"])
def test_stdout_newlines_sp(tmp_path, monkeypatch, newline):
    """When using ``--stdout``, newlines are not duplicated.

    See: https://github.com/akaihola/darker/issues/604

    """
    import subprocess
    monkeypatch.chdir(tmp_path)

    code = f"import collections{newline}import sys{newline}".encode()
    Path("new-file.py").write_bytes(code)
    cp = subprocess.run(["darker", "--stdout", "--isort", "--stdin-filename=new-file.py", "-"],
                   input=code,
                   stdout=subprocess.PIPE,
                   check=True,
                   )

    expect = f"import collections{newline}import sys{newline}".encode()
    assert cp.stdout == expect

So the mocking is behaving differently to a real pipe here.

Similarly the output from capsysbinary is not line-end translated, whereas the stdout from subprocess.PIPE is.

shane-kearns commented 3 months ago

A trivial python program:

print("\r\n")

Will print double newlines to a pipe on windows. However, the pytest with captured I/O does not:

def test_newlines2(capfdbinary):
    print("\r\n", end="")
    assert capfdbinary.readouterr().out == b"\r\n"

So the correct fix would be in print_source, replacing print(new.string, end="") with sys.stdout.buffer.write(new.string.encode())

I don't know how to test within a single process, as pytest's mocking and capturing disables the line end conversion that happens on windows when using sys.stdout.write or print.

akaihola commented 3 months ago

@shane-kearns, I added your proposed fix. Could you test this and review my changes?

akaihola commented 1 month ago

@shane-kearns, I rebased the Darker "double newlines" fix on master, and it's scheduled for release 3.0.1.

You're still assigned as the reviewer for the PR (#605) – feel free to take a look if you have time. I'll also look for other reviewers among recent contributors just in case you're too busy right now.