-
-
Notifications
You must be signed in to change notification settings - Fork 611
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
pip-compile
: Use the same newline string already found in relevant files (if impossible use LF), or override with --force-lf-newlines
#1584
Conversation
24d7e29
to
46a5b15
Compare
7903d76
to
c5470a5
Compare
c5470a5
to
e64b217
Compare
e64b217
to
91bd957
Compare
91bd957
to
eb9ade0
Compare
eb9ade0
to
79af990
Compare
09344e4
to
1cb6997
Compare
1cb6997
to
efa88f7
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
piptools/scripts/compile.py
Outdated
@@ -159,6 +159,12 @@ def _get_default_option(option_name: str) -> Any: | |||
"Will be derived from input file otherwise." | |||
), | |||
) | |||
@click.option( | |||
"--force-unix-newlines", |
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 don't like the name of this flag because it implies that LF is only for Unix, however other operating systems are gradually coming around to LF being the only option (windows)
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'm going to push here an update based on changes in the other PR. What do you think the flag could be called instead?
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.
That's part of the reason I prefer the other PR, the CLI is significantly more understandable at the cost of a tiny number of extra lines, and nobody needs to bikeshed this name
The best I can come up with is:
"--force-unix-newlines", | |
"--force-lf-line-separator", |
"--force-unix-newlines", | |
"--newline-is-lf", |
--newline=lf
is so readable and obvious, it just has a drawback that it implies the existence of --newline=crlf
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.
newline-is-lf
sounds descriptive, but we're being prescriptive. What about:
write-lf
force-lf
write-lf-newlines
force-lf-newlines
?
piptools/scripts/compile.py
Outdated
@@ -159,6 +159,12 @@ def _get_default_option(option_name: str) -> Any: | |||
"Will be derived from input file otherwise." | |||
), | |||
) | |||
@click.option( | |||
"--force-unix-newlines", |
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.
That's part of the reason I prefer the other PR, the CLI is significantly more understandable at the cost of a tiny number of extra lines, and nobody needs to bikeshed this name
The best I can come up with is:
"--force-unix-newlines", | |
"--force-lf-line-separator", |
"--force-unix-newlines", | |
"--newline-is-lf", |
--newline=lf
is so readable and obvious, it just has a drawback that it implies the existence of --newline=crlf
efa88f7
to
1b4a0d4
Compare
@ssbarnea I understand you don't really want this in any form, but if it does happen in this (single flag) form, would you like to suggest a better flag name? EDIT: In this PR it's currently |
1b4a0d4
to
ae15c4d
Compare
pip-compile
: Use the same newline string already found in relevant files (if impossible, use LF), or override with --force-lf-newlines
pip-compile
: Use the same newline string already found in relevant files (if impossible, use LF), or override with --force-lf-newlines
pip-compile
: Use the same newline string already found in relevant files (if impossible use LF), or override with --force-lf-newlines
Need to use a local repo for the `--force-lf-newlines` option, at least until jazzband/pip-tools#1584 can be merged.
Need to point to a pip-tools fork for the `--force-lf-newlines` option until jazzband/pip-tools#1584 can be merged.
ae15c4d
to
bed2edb
Compare
Looks like jazzband/pip-tools#1584 was force-pushed a while back and the reference to the previously used revision hash was lost.
89fa7ee
to
ded5950
Compare
- 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 <tagrain@gmail.com>
The default behavior remains identical to what was --newline=preserve.
dd85a3b
to
d247f43
Compare
I think that this change was obsoleted by #1652 so closing. If not, please rebase and reopen. |
This changes
pip-compile
's default behavior for writing newlines (1), and adds an overriding flag (2).(1)
By default,
pip-compile
will determine a newline string to use based on a "preserve" strategy:\n
)(2)
pip-compile
gains a flag:--force-lf-newlines
, which ensures LF is used in the output file.This aims to address #1448.
Contributor checklist
Maintainer checklist
backwards incompatible
,feature
,enhancement
,deprecation
,bug
,dependency
,docs
orskip-changelog
as they determine changelog listing.