Add `--newline=[LF|CRLF|native|preserve]` option to `pip-compile` #1652

AndydeCleyre · 2022-07-03T03:10:07Z

pip-compile gains an option with four valid choices: --newline=[LF|CRLF|native|preserve],
which can be used to override the guessed newline character used in the output file. The default is preserve, which tries to be consistent with an existing output file, or input file, or falls back to LF, in that order.

This aims to address #1448.

Contributor checklist

Provided the tests for the changes.
Assure PR title is short, clear, and good to be included in the user-oriented changelog

Maintainer checklist

Assure one of these labels is present: backwards incompatible, feature, enhancement, deprecation, bug, dependency, docs or skip-changelog as they determine changelog listing.
Assign the PR to an existing or new milestone for the target version (following Semantic Versioning).

AndydeCleyre · 2022-07-03T03:37:19Z

See also: #1584, which adds a commit on top of this, reducing the options to a single ~~--force-unix-newlines~~ --force-lf-newlines flag.

graingert · 2022-07-15T19:21:24Z

My review was requested on this, and I really want the feature and the code looks great to me - but I don't feel like I'm in a position to approve this PR

graingert · 2022-07-18T15:48:10Z

@ssbarnea are you happy for me to review this PR?

ssbarnea · 2022-07-18T16:33:33Z

@graingert If all those conditions are met, feel free to make your review official. As I said, I am bit of 0.5 on this, so we will need two approvals to merge it. I do not want to alienate the author, but at the same time I cannot refrain from seeing this feature as adding useless complexity, as use of git attributes looks like a valid workaround.

AndydeCleyre · 2022-07-18T17:06:18Z

@ssbarnea Please don't worry about alienating me; this is not even a feature I'm interested in personally. And don't forget there's the slightly simpler alternative at #1584.

piptools/scripts/compile.py

graingert · 2022-07-18T21:16:51Z

I cannot refrain from seeing this feature as adding useless complexity, as use of git attributes looks like a valid workaround

I do see this as additional complexity however I think this PR has really hit the nail on the head of balancing usability and backwards compatibility. In addition it's solving a tough problem that has been frustrating me and the people I introduce to pip-tools and other CLI tools that edit config so I plan to reuse this pattern and point to this implementation as an example

- Use it in writer to override the line sep used in output, using io.TextIOWrapper - Set default to preserve, which checks the output file, then the input file(s) - Falls back to LF if that's not possible

- skip decode to save time - catch FileNotFoundError to avoid potential race condition Co-authored-by: Thomas Grainger <[email protected]>

webknjaz · 2022-10-05T16:51:54Z

piptools/writer.py

-                log.info(line)
-                self.dst_file.write(unstyle(line).encode())
-                self.dst_file.write(os.linesep.encode())
+        if not self.dry_run:


FWIW this could be cleaner if implemented with a context manager conditionally returning a dummy I/O object.

done here #1698

webknjaz · 2022-10-05T16:53:59Z

tests/test_cli_compile.py

+    if gen_hashes:
+        opts += ("--generate-hashes",)
+
+    with open("requirements.in", "w") as req_in:


I feel like with modern Pythons it'd be nicer if Pathlib.write_text() was used. But more importantly, why don't you use the built-in tmp_path fixture to write temporary files?

Feel free to make any changes you like, and thanks! This change isn't for me personally, and I have COVID right now and can't bring my full focus to the task.

I'll try and make a PR with this fix in!

done here: #1694

webknjaz

Aside for a few refactoring suggestions, this LGTM.

AndydeCleyre mentioned this pull request Jul 3, 2022

Add --newline=[LF|CRLF|native|preserve] option to compile, to override the line separator characters used #1491

Closed

4 tasks

AndydeCleyre requested a review from graingert July 3, 2022 03:37

AndydeCleyre force-pushed the feature/linesep-textiowrapper branch 2 times, most recently from 3d1f27a to b8df966 Compare July 18, 2022 15:47

ssbarnea added the enhancement Improvements to functionality label Jul 18, 2022

graingert reviewed Jul 18, 2022

View reviewed changes

piptools/scripts/compile.py Outdated Show resolved Hide resolved

graingert requested changes Jul 18, 2022

View reviewed changes

piptools/scripts/compile.py Outdated Show resolved Hide resolved

AndydeCleyre force-pushed the feature/linesep-textiowrapper branch 2 times, most recently from 6b372f0 to 4250f1e Compare July 18, 2022 20:37

graingert approved these changes Jul 18, 2022

View reviewed changes

AndydeCleyre mentioned this pull request Jul 19, 2022

Add a way to choose line endings in compiled files #1448

Open

AndydeCleyre force-pushed the feature/linesep-textiowrapper branch from 4250f1e to 6571d55 Compare August 15, 2022 13:43

AndydeCleyre force-pushed the feature/linesep-textiowrapper branch from 6571d55 to 1bec196 Compare October 1, 2022 20:01

AndydeCleyre and others added 3 commits October 3, 2022 11:14

Add --newline [LF|CRLF|native|preserve] option to compile

2af5c3e

- Use it in writer to override the line sep used in output, using io.TextIOWrapper - Set default to preserve, which checks the output file, then the input file(s) - Falls back to LF if that's not possible

Add tests for --newline option

16ce7ca

Move linesep determination to a dedicated function, with improvements

a55c423

- skip decode to save time - catch FileNotFoundError to avoid potential race condition Co-authored-by: Thomas Grainger <[email protected]>

AndydeCleyre force-pushed the feature/linesep-textiowrapper branch from 1bec196 to a55c423 Compare October 3, 2022 15:14

webknjaz reviewed Oct 5, 2022

View reviewed changes

webknjaz approved these changes Oct 5, 2022

View reviewed changes

ssbarnea added this to the 6.10.0 milestone Oct 5, 2022

Merge branch 'master' into feature/linesep-textiowrapper

eb46521

ssbarnea enabled auto-merge (squash) October 5, 2022 19:43

ssbarnea merged commit eff8476 into jazzband:master Oct 5, 2022

ssbarnea mentioned this pull request Oct 6, 2022

pip-compile: Use the same newline string already found in relevant files (if impossible use LF), or override with --force-lf-newlines #1584

Closed

4 tasks

graingert mentioned this pull request Oct 7, 2022

use tmp_path for test_override_newline #1694

Merged

4 tasks

atugushev added the cli Related to command line interface things label Nov 11, 2022

jeffwidman mentioned this pull request Nov 21, 2022

Investigate using the --newline=[LF|CRLF|native|preserve] option in pip-compile dependabot/dependabot-core#6186

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `--newline=[LF|CRLF|native|preserve]` option to `pip-compile` #1652

Add `--newline=[LF|CRLF|native|preserve]` option to `pip-compile` #1652

AndydeCleyre commented Jul 3, 2022 •

edited by atugushev

Loading

AndydeCleyre commented Jul 3, 2022 •

edited

Loading

graingert commented Jul 15, 2022

graingert commented Jul 18, 2022

ssbarnea commented Jul 18, 2022

AndydeCleyre commented Jul 18, 2022

graingert commented Jul 18, 2022

webknjaz Oct 5, 2022

graingert Oct 7, 2022 •

edited

Loading

webknjaz Oct 5, 2022

AndydeCleyre Oct 5, 2022

graingert Oct 7, 2022

graingert Oct 7, 2022

webknjaz left a comment

Add --newline=[LF|CRLF|native|preserve] option to pip-compile #1652

Add --newline=[LF|CRLF|native|preserve] option to pip-compile #1652

Conversation

AndydeCleyre commented Jul 3, 2022 • edited by atugushev Loading

Contributor checklist

Maintainer checklist

AndydeCleyre commented Jul 3, 2022 • edited Loading

graingert commented Jul 15, 2022

graingert commented Jul 18, 2022

ssbarnea commented Jul 18, 2022

AndydeCleyre commented Jul 18, 2022

graingert commented Jul 18, 2022

webknjaz Oct 5, 2022

Choose a reason for hiding this comment

graingert Oct 7, 2022 • edited Loading

Choose a reason for hiding this comment

webknjaz Oct 5, 2022

Choose a reason for hiding this comment

AndydeCleyre Oct 5, 2022

Choose a reason for hiding this comment

graingert Oct 7, 2022

Choose a reason for hiding this comment

graingert Oct 7, 2022

Choose a reason for hiding this comment

webknjaz left a comment

Choose a reason for hiding this comment

Add `--newline=[LF|CRLF|native|preserve]` option to `pip-compile` #1652

Add `--newline=[LF|CRLF|native|preserve]` option to `pip-compile` #1652

AndydeCleyre commented Jul 3, 2022 •

edited by atugushev

Loading

AndydeCleyre commented Jul 3, 2022 •

edited

Loading

graingert Oct 7, 2022 •

edited

Loading