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

(🎁) Preserve crlf line endings in blackd #3257

Merged
merged 1 commit into from Oct 4, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGES.md
Expand Up @@ -36,7 +36,7 @@

### _Blackd_

<!-- Changes to blackd -->
- Windows style (CRLF) newlines will be preserved (#3257).

### Integrations

Expand Down
5 changes: 5 additions & 0 deletions docs/the_black_code_style/current_style.md
Expand Up @@ -406,6 +406,11 @@ file that are not enforced yet but might be in a future version of the formatter
- use variable annotations instead of type comments, even for stubs that target older
versions of Python.

### Line endings

_Black_ will normalize line endings (`\n` or `\r\n`) based on the first line ending of
the file.

## Pragmatism

Early versions of _Black_ used to be absolutist in some respects. They took after its
Expand Down
7 changes: 7 additions & 0 deletions src/blackd/__init__.py
Expand Up @@ -133,6 +133,13 @@ async def handle(request: web.Request, executor: Executor) -> web.Response:
executor, partial(black.format_file_contents, req_str, fast=fast, mode=mode)
)

# Preserve CRLF line endings
if req_str[req_str.find("\n") - 1] == "\r":
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to mix \n and \r\n newlines in a single file? If so, this could change the contents of string literals with \n newlines in them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is what normal black does currently to files, although I'm not certain.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I just checked with a mixed file in normal black, and it uses the first newline to rewrite the rest of the file.

I think formatting while preserving mixed newlines would be extremely difficult and probably a giant waste of effort.

formatted_str = formatted_str.replace("\n", "\r\n")
# If, after swapping line endings, nothing changed, then say so
if formatted_str == req_str:
raise black.NothingChanged

# Only output the diff in the HTTP response
only_diff = bool(request.headers.get(DIFF_HEADER, False))
if only_diff:
Expand Down
11 changes: 11 additions & 0 deletions tests/test_black.py
Expand Up @@ -1286,6 +1286,17 @@ def test_preserves_line_endings_via_stdin(self) -> None:
if nl == "\n":
self.assertNotIn(b"\r\n", output)

def test_normalize_line_endings(self) -> None:
with TemporaryDirectory() as workspace:
test_file = Path(workspace) / "test.py"
for data, expected in (
(b"c\r\nc\n ", b"c\r\nc\r\n"),
(b"l\nl\r\n ", b"l\nl\n"),
):
test_file.write_bytes(data)
ff(test_file, write_back=black.WriteBack.YES)
self.assertEqual(test_file.read_bytes(), expected)

def test_assert_equivalent_different_asts(self) -> None:
with self.assertRaises(AssertionError):
black.assert_equivalent("{}", "None")
Expand Down
17 changes: 17 additions & 0 deletions tests/test_blackd.py
Expand Up @@ -209,3 +209,20 @@ async def test_cors_headers_present(self) -> None:
response = await self.client.post("/", headers={"Origin": "*"})
self.assertIsNotNone(response.headers.get("Access-Control-Allow-Origin"))
self.assertIsNotNone(response.headers.get("Access-Control-Expose-Headers"))

@unittest_run_loop
async def test_preserves_line_endings(self) -> None:
for data in (b"c\r\nc\r\n", b"l\nl\n"):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we also test:

  • first is only n, second is rn, and the corresponding expectation
  • the other way around

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what you mean, could you provide an example please.

"a\na\r\n"

Like this?

If so, should the same test be added to normal black(tbh it might already), and should we document the behaviour as well?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep exactly, sorry for the confusion 😅 and a\r\na\n At a quick glance, I couldn't find a test with mixed line endings. Only ones like the one you wrote here. But it would be nice, and a short mention on documentation couldn't hurt either, agreed 👍 Just to be explicit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have made a sus discovery, black will not normalize newlines if the rest of the file is unchanged. Should I make blackd behave exactly the same? (I think always normalizing is arguably better).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added tests for both blackd and black, and updated the docs.

# test preserved newlines when reformatted
response = await self.client.post("/", data=data + b" ")
self.assertEqual(await response.text(), data.decode())
# test 204 when no change
response = await self.client.post("/", data=data)
self.assertEqual(response.status, 204)

@unittest_run_loop
async def test_normalizes_line_endings(self) -> None:
for data, expected in ((b"c\r\nc\n", "c\r\nc\r\n"), (b"l\nl\r\n", "l\nl\n")):
response = await self.client.post("/", data=data)
self.assertEqual(await response.text(), expected)
self.assertEqual(response.status, 200)