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

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

Open
Jackenmen opened this issue Jul 8, 2021 · 24 comments
Open

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

Jackenmen opened this issue Jul 8, 2021 · 24 comments
Labels
feature Request for a new feature writer Related to results output writer component

Comments

@Jackenmen
Copy link

Jackenmen commented Jul 8, 2021

What's the problem this feature will solve?

I'm trying to use the pip-compile hook with my existing pre-commit configuration but one of the hooks I have is a mixed line ending auto-fixer (which I use to ensure all files in the repository use LF for line endings):

repos:
  - repo: https://github.com/jazzband/pip-tools
    rev: 6.2.0
    hooks:
      - id: pip-compile
  - repo: https://github.com/pre-commit/pre-commit-hooks
    rev: v3.4.0
    hooks:
      - id: mixed-line-ending
        args:
          - "--fix=lf"

This configuration causes issues whenever I try to touch a requirements file.

Describe the solution you'd like

A way to set the line endings used for the generated requirements.txt file. The alternative would be for pip-compile to always use \n but then I'm guessing someone else might complain about it in the future like I'm doing now :)

Alternative Solutions

I can work around this issue by excluding requirement files from the check but this doesn't really solve it as I still end up with requirements files ending with CRLF, it just allows me to continue to use both hooks.

Another solution would be a way for --dry-run to return non-zero exit code when the file would have any dependencies changed (like what #882 and #1215 suggest) but this doesn't fully solve it either as then you lose the benefit of pip-compile auto-fixing the issue during pre-commit run.

Additional context

I'm using on Windows - os.linesep is equal to "\r\n" there

@atugushev atugushev added feature Request for a new feature writer Related to results output writer component labels Jul 17, 2021
@graingert
Copy link
Member

graingert commented Jul 19, 2021

Imho pip-compile should reuse whatever line separator it finds first in requirements.txt, and fall back to the first line separator it finds in requirements.in

@AndydeCleyre
Copy link
Contributor

This configuration causes issues whenever I try to touch a requirements file.

Can you please elaborate on the problem? Do you not have a guaranteed order of hook runs (to ensure the line-ending hook runs last)?

@Jackenmen
Copy link
Author

Run this on Windows (due to os.linesep being equal to "\r\n" there):

python -m pip install -U pre-commit
git clone https://github.com/jack1142/pip-tools-issue-1448
cd pip-tools-issue-1448
pre-commit run --all-files

See the output:

pip-compile..............................................................Passed
Mixed line ending........................................................Failed
- hook id: mixed-line-ending
- exit code: 1

requirements.txt: fixed mixed line endings

The problem is that pip-compile reintroduces the issue with line endings whenever it runs therefore causing the mixed-line-ending hook to be triggered whenever requirements.in/txt is modified in the commit or when using pre-commit run --all-files. The guaranteed order of hook runs doesn't matter here, pre-commit will fail the commit if any of the hooks fail and that's what is happening here.

@AndydeCleyre
Copy link
Contributor

I've done a first try at https://github.com/AndydeCleyre/pip-tools/tree/feature/linesep -- no tests yet, no PR yet.

@jack1142 can you test out that branch in your pre-commit setup, and add the arg --newline=lf to the config?

@Jackenmen
Copy link
Author

@AndydeCleyre Seems to work as intended.

@AndydeCleyre
Copy link
Contributor

If any Windows users are reading this who haven't already responded to the poll about the best default behavior, please go have a look, thanks!

@AndydeCleyre

This comment was marked as outdated.

@AndydeCleyre
Copy link
Contributor

AndydeCleyre commented Feb 4, 2022

Checking in:

There is a PR in good condition that addresses this in a flexible way, #1491 #1652, but an alternative approach has been proposed to just use LF no matter what.

Either solution Works For Me, but I'm requesting comments from Windows users and those with mixed OS dev teams.

@AndydeCleyre
Copy link
Contributor

AndydeCleyre commented Feb 15, 2022

@ssbarnea @jack1142

Maybe the move forward is to collpase #1491 #1652 into a single new flag, --preserve-newlines (or similar):

  • without it, always use LF
  • with it, and existing newlines can be detected, use those
  • with it, and no existing newlines are detected, use LF

What do you think?

@graingert
Copy link
Member

In the usecase stated in the issue "preserve" is a better choice. The pre-commit hook https://github.com/pre-commit/pre-commit-hooks#mixed-line-ending in combination with "preserve" will handle this

@AndydeCleyre
Copy link
Contributor

So more like an --LF flag is called for?

@Jackenmen
Copy link
Author

@ssbarnea @jack1142

Maybe the move forward is to collpase #1491 into a single new flag, --preserve-newlines (or similar):

  • without it, always use LF
  • with it, and existing newlines can be detected, use those
  • with it, and no existing newlines are detected, use LF

What do you think?

Seems fine to me though Thomas commented in the PR that preserve should be default (which I'm not sure I agree with but it doesn't matter to me since it's going to end up as LF in both cases anyway)

@ssbarnea
Copy link
Member

I am for LF implicit and having a single opt-in preserve option for those looking for one. That keeps the added complexity out of the execution path for most users.

TBH, I do find any developer that does not configure git to force LF as looking for trouble, especially as most tools work well with LF, even on Windows. Just think about mounting and cross platform issues.

@MarkKoz
Copy link

MarkKoz commented Feb 16, 2022

TBH, I do find any developer that does not configure git to force LF as looking for trouble, especially as most tools work well with LF, even on Windows. Just think about mounting and cross platform issues.

In my experience, trying to force LF has resulted in similar issues, where some tool edits a file and naïvely uses CRLF. Git does a good job at converting back to LF on check in, so there shouldn't be a need to force a specific line ending on check out.

@AndydeCleyre
Copy link
Contributor

AndydeCleyre commented Mar 2, 2022

Poll:

  • 👀 preserve by default, and add --force-unix-newlines
  • 🎉 preserve by default, and add --newline=[LF|CRLF|native|preserve]
  • 🚀 LF by default, and add --preserve-newlines
  • 😆 preserve, with no alternatives
  • 😕 none of the above

You can vote for one or more if you're feeling flexible.

@MarkKoz
Copy link

MarkKoz commented Mar 2, 2022

How about preserve by default, and fall back to the line endings in requirements.in / setup.py / other input file as someone suggested? Don't add any option to convert the line endings. I'm not strongly against the conversion options, but they seem redundant. They'd be extra work to support and there'd be more cognitive burden to understand the extra options.

With just the preserve by default behaviour, there will already be several options to force a specific line ending:

  1. Don't use the undesired line ending to begin with in the input. If one wants LF for pip-tools's output, then surely they are already using LF for the input files. And if this is the case, then LF will get preserved by pip-tools.
  2. Use a pre-commit hook to convert them
  3. Rely on git to convert them

Are these insufficient?

@AndydeCleyre
Copy link
Contributor

AndydeCleyre commented Mar 3, 2022

How about preserve by default, and fall back to the line endings in requirements.in / setup.py / other input file as someone suggested?

To be clear, the "preserve newlines" behavior as implemented in alternative PRs #1491 #1652 and #1584 does include that fall-back-to-input bit.

If one wants LF for pip-tools's output, then surely they are already using LF for the input files.

I'm less sure, and welcome a description of any real user cases violating this prediction.

Theoretically some users may not be using git...

Anyway I'll add an option to the poll:

  • 😆 preserve, with no alternatives

@MarkKoz
Copy link

MarkKoz commented Mar 3, 2022

Theoretically some users may not be using git...

Yeah, fair enough. The first point is indeed the strongest one I think.

@graingert
Copy link
Member

preserve, with no alternatives

means pip-compile crashes when used with data from stdin?

@AndydeCleyre
Copy link
Contributor

preserve, with no alternatives

means pip-compile crashes when used with data from stdin?

No, it means try to use newline from existing output, and failing that, try to use newline from input file, and failing that, use LF.

@AndydeCleyre
Copy link
Contributor

Are all users affected by this issue equally satisfied by alternatives #1652 and #1584?

@Jackenmen
Copy link
Author

#1584 could use a PR description as the difference between these PRs is not immediately clear.

Based on the code read, it seems that #1584 has the behavior of --newline=LF (from #1652) when --force-lf-newlines is used and --newline=preserve otherwise (with the default being LF for new files). Since I personally am most interested in forcing LF, this does satisfy my needs.
I consider preserve the second best option since if the file is already using LF then it's just going to reuse LF and if the file doesn't already exist then it will use LF as default.

I personally did not see the need for --newline=CRLF or the worst of all - --newline=native which makes the output dependent on the OS you're using.

@MarkKoz
Copy link

MarkKoz commented Jul 19, 2022

The most important thing to me is that pip-tools stops producing files with mixed line endings. I don't care what the line ending is as long as its consistent. Both PRs seem to accomplish this, but I lean towards #1584 since it's simpler. In fact, I don't think a way to force line endings is needed (as outlined in #1448 (comment), there are alternatives to accomplish this). Not a dealbreaker though; it's an extra argument but at least it doesn't seem to have much impact on the complexity of the implementation.

@graingert
Copy link
Member

--newline=native is useful for backwards compatibility
--newline=CRLF is useful for some users who are Windows only

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Request for a new feature writer Related to results output writer component
Projects
None yet
Development

No branches or pull requests

6 participants