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

Avoid double newlines with --stdout #605

Merged
merged 2 commits into from
Oct 8, 2024
Merged

Conversation

akaihola
Copy link
Owner

@akaihola akaihola commented Aug 1, 2024

Fixes #604

@akaihola akaihola added the bug Something isn't working label Aug 1, 2024
@akaihola akaihola self-assigned this Aug 1, 2024
@akaihola akaihola marked this pull request as draft August 1, 2024 07:03
@akaihola
Copy link
Owner Author

akaihola commented Aug 1, 2024

@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
Copy link

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
Copy link

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 akaihola force-pushed the stdout-double-newlines branch 5 times, most recently from b90d9a2 to a67615c Compare August 8, 2024 21:26
@akaihola akaihola marked this pull request as ready for review August 9, 2024 05:33
@akaihola akaihola force-pushed the stdout-double-newlines branch from fd5fce4 to 9706429 Compare August 9, 2024 05:36
@akaihola
Copy link
Owner Author

akaihola commented Aug 9, 2024

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

@akaihola akaihola force-pushed the stdout-double-newlines branch from 9706429 to 769ef24 Compare September 24, 2024 19:02
@akaihola akaihola force-pushed the stdout-double-newlines branch from 769ef24 to d55f02f Compare October 6, 2024 13:32
@akaihola
Copy link
Owner Author

akaihola commented Oct 6, 2024

@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.

@akaihola akaihola merged commit c70c7fa into master Oct 8, 2024
37 checks passed
@akaihola akaihola deleted the stdout-double-newlines branch October 8, 2024 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Development

Successfully merging this pull request may close these issues.

--stdout doubles newlines
2 participants