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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 4 commits into from
Oct 5, 2022
Merged

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

merged 4 commits into from
Oct 5, 2022

Conversation

AndydeCleyre
Copy link
Contributor

@AndydeCleyre AndydeCleyre commented Jul 3, 2022 •
edited by atugushev
Loading

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 AndydeCleyre mentioned this pull request Jul 3, 2022
Closed
4 tasks
@AndydeCleyre AndydeCleyre changed the title Add --newline=[LF|CRLF|native|preserve] option to compile, to override the line separator characters used Add --newline=[LF|CRLF|native|preserve] option to compile, to override the line separator characters written Jul 3, 2022
@AndydeCleyre AndydeCleyre changed the title Add --newline=[LF|CRLF|native|preserve] option to compile, to override the line separator characters written Add --newline=[LF|CRLF|native|preserve] to compile, to override the line separator characters written Jul 3, 2022
@AndydeCleyre
Copy link
Contributor Author

AndydeCleyre commented Jul 3, 2022 •
edited
Loading

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

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

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

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

@ssbarnea
Copy link
Member

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

@ssbarnea ssbarnea added the enhancement Improvements to functionality label Jul 18, 2022
@AndydeCleyre
Copy link
Contributor Author

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

graingert
graingert requested changes Jul 18, 2022
View reviewed changes
piptools/scripts/compile.py Outdated Show resolved Hide resolved
@AndydeCleyre AndydeCleyre force-pushed the feature/linesep-textiowrapper branch 2 times, most recently from 6b372f0 to 4250f1e Compare July 18, 2022 20:37
@graingert
Copy link
Member

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

@AndydeCleyre AndydeCleyre mentioned this pull request Jul 19, 2022
Open
AndydeCleyre and others added 3 commits October 3, 2022 11:14
@AndydeCleyre
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
@AndydeCleyre
Add tests for --newline option
16ce7ca
@AndydeCleyre @graingert
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 <tagrain@gmail.com>
webknjaz
webknjaz reviewed Oct 5, 2022
View reviewed changes
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:
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

@graingert graingert Oct 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

done here #1698

webknjaz
webknjaz reviewed Oct 5, 2022
View reviewed changes
tests/test_cli_compile.py
if gen_hashes:
opts += ("--generate-hashes",)

with open("requirements.in", "w") as req_in:
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

done here: #1694

webknjaz
webknjaz approved these changes Oct 5, 2022
View reviewed changes
Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

Aside for a few refactoring suggestions, this LGTM.

@ssbarnea ssbarnea added this to the 6.10.0 milestone Oct 5, 2022
@ssbarnea ssbarnea enabled auto-merge (squash) October 5, 2022 19:43
@ssbarnea ssbarnea changed the title Add --newline=[LF|CRLF|native|preserve] to compile, to override the line separator characters written Add --newline=[LF|CRLF|native|preserve] to compile Oct 5, 2022
@ssbarnea ssbarnea merged commit eff8476 into jazzband:master Oct 5, 2022
@ssbarnea ssbarnea mentioned this pull request Oct 6, 2022
Closed
4 tasks
@graingert graingert mentioned this pull request Oct 7, 2022
Merged
4 tasks
@atugushev atugushev changed the title Add --newline=[LF|CRLF|native|preserve] to compile Add --newline=[LF|CRLF|native|preserve] to pip-compile Nov 10, 2022
@atugushev atugushev changed the title Add --newline=[LF|CRLF|native|preserve] to pip-compile Add --newline=[LF|CRLF|native|preserve] option to pip-compile Nov 10, 2022
@atugushev atugushev added the cli Related to command line interface things label Nov 11, 2022
@jeffwidman jeffwidman mentioned this pull request Nov 21, 2022
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@graingert graingert graingert approved these changes

@webknjaz webknjaz webknjaz approved these changes

Assignees
No one assigned
Labels
cli Related to command line interface things enhancement Improvements to functionality
Projects
None yet
Milestone
6.10.0
Development

Successfully merging this pull request may close these issues.
5 participants
@AndydeCleyre @graingert @ssbarnea @webknjaz @atugushev