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

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

Closed

Conversation

AndydeCleyre
Copy link
Contributor

@AndydeCleyre AndydeCleyre commented Feb 16, 2022

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:

  • if a pre-existing output file has a newline, use that
  • otherwise, if an input file has a newline, use that
  • otherwise, use LF (\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
  • 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).

@graingert

This comment was marked as resolved.

@AndydeCleyre

This comment was marked as resolved.

@graingert

This comment was marked as resolved.

@@ -159,6 +159,12 @@ def _get_default_option(option_name: str) -> Any:
"Will be derived from input file otherwise."
),
)
@click.option(
"--force-unix-newlines",
Copy link
Member

@graingert graingert Jul 18, 2022

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)

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 going to push here an update based on changes in the other PR. What do you think the flag could be called instead?

Copy link
Member

@graingert graingert Jul 18, 2022

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:

Suggested change
"--force-unix-newlines",
"--force-lf-line-separator",
Suggested change
"--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

Copy link
Contributor Author

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

?

@@ -159,6 +159,12 @@ def _get_default_option(option_name: str) -> Any:
"Will be derived from input file otherwise."
),
)
@click.option(
"--force-unix-newlines",
Copy link
Member

@graingert graingert Jul 18, 2022

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:

Suggested change
"--force-unix-newlines",
"--force-lf-line-separator",
Suggested change
"--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

@AndydeCleyre
Copy link
Contributor Author

AndydeCleyre commented Jul 18, 2022

@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 --force-lf-newlines.

@AndydeCleyre AndydeCleyre changed the title Feature/linesep slim pip-compile: Use the same newline string already found in relevant files (if impossible, use LF), or override with --force-lf-newlines Jul 19, 2022
@AndydeCleyre AndydeCleyre changed the title 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 Jul 19, 2022
@AndydeCleyre AndydeCleyre marked this pull request as ready for review July 26, 2022 15:32
nuztalgia added a commit to nuztalgia/botstrap that referenced this pull request Aug 9, 2022
Need to use a local repo for the `--force-lf-newlines` option, at least until jazzband/pip-tools#1584 can be merged.
nuztalgia added a commit to nuztalgia/botstrap that referenced this pull request Aug 9, 2022
Need to point to a pip-tools fork for the `--force-lf-newlines` option until jazzband/pip-tools#1584 can be merged.
nuztalgia added a commit to nuztalgia/botstrap that referenced this pull request Sep 10, 2022
Looks like jazzband/pip-tools#1584 was force-pushed a while back and the reference to the previously used revision hash was lost.
@AndydeCleyre AndydeCleyre force-pushed the feature/linesep-slim branch 2 times, most recently from 89fa7ee to ded5950 Compare October 3, 2022 15:17
AndydeCleyre and others added 4 commits October 5, 2022 18:48
- 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.
@ssbarnea
Copy link
Member

ssbarnea commented Oct 6, 2022

I think that this change was obsoleted by #1652 so closing. If not, please rebase and reopen.

@ssbarnea ssbarnea closed this Oct 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants