-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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"): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we also test:
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep exactly, sorry for the confusion 😅 and There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) |
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.