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

yarn uses UNIX line endings on all systems (also Windows) #1061

Closed
ahelmberger opened this issue Oct 14, 2016 · 14 comments
Closed

yarn uses UNIX line endings on all systems (also Windows) #1061

ahelmberger opened this issue Oct 14, 2016 · 14 comments

Comments

@ahelmberger
Copy link

ahelmberger commented Oct 14, 2016

Do you want to request a feature or report a bug?
Depends on the point of view

What is the current behavior?
yarn uses UNIX line endings on all systems

If the current behavior is a bug, please provide the steps to reproduce.
When generating lockfile

What is the expected behavior?
It should probably use the system EOL (e.g. '\r\n' on Windows), or at least check what's currently used in an existing lockfile otherwise it annoys users by creating changes in version control systems due to line endings

Please mention your node.js, yarn and operating system version.
Windows 7 64bit
node v6.6.0
yarn 0.15.1

@Daniel15
Copy link
Member

Daniel15 commented Oct 14, 2016

Good catch - This should use os.EOL (or determine the ending to use based on the contents of the file) instead of making assumptions.

@ljharb
Copy link

ljharb commented Oct 14, 2016

Determining the existing endings to minimize diffs is good, but a default to LF otherwise imo is a better idea. That way projects that are used cross-platform can also minimize diffs.

@ahelmberger
Copy link
Author

IMHO, having LF as the default line ending also for Windows is not the best option. It's just not what Windows users expect and not how most other tools behave. In my experience, working in cross-platform teams for a while now and trying out different conventions, it feels best for everyone (and leads to zero diffs) if tools respect the platforms they run on, and Git does the job ob CRLF->LF conversions (as that is basically the de-facto-standard setting for Git on Windows).

@Daniel15
Copy link
Member

a default to LF otherwise imo is a better idea

Disagree - In a primarily Windows team, you'd probably want Windows line endings everywhere. Maybe it could be configurable. As @ahelmberger mentioned, Git converts CRLF -> LF on commit (and LF -> CRLF on checkout) automatically anyways.

@ljharb
Copy link

ljharb commented Oct 16, 2016

If that's true, then since yarn.lock is committed, wouldn't everyone be looking at LF-only diffs anyways? (Or CRLF, as the case may be)

@ljharb
Copy link

ljharb commented Oct 16, 2016

In my past experience all my Windows tools handled LF just fine - is this no longer the case with popular tools?

@ahelmberger
Copy link
Author

@ljharb It's not that Windows tools couldn't handle LFs, it's just annoying to have a dirty working directory after a clean checkout and yarn install.

@bterlson
Copy link

Agree that dirtying due to improper line endings and mixing line endings is bad, but I want to suggest that however this issue is fixed that it not put CR's in my pristine LF-only files. Some windows users like me prefer never to see CR anywhere (eg. my git is configured to checkout LF, check in LF).

@olingern
Copy link
Contributor

olingern commented Oct 23, 2016

I got to tinkering with this and have the beginnings of something. Still rough around the edges, and needs tests, but I should have a PR for this over the next day or so.

Good catch - This should use os.EOL (or determine the ending to use based on the contents of the file) instead of making assumptions.

Essentially, my idea was to build on what @Daniel15 mentioned with os.EOL, but provide some flexibility for users like, @bterlson. So, this commit defaults to the OS's EOL, unless a user passes in a flag,
--eol to override the default.

For example:

  • CR \r
    • yarn --eol=CR
  • CRLF \r\n
    • yarn --eol=CRLF
  • LF \n
    • yarn --eol=LF

Thoughts on this?

@Daniel15, @kittens - I'm probably not keen on is each instance when I lockfile may change, and all commands that might trigger a needed change / parsing. Any feedback is much welcomed!

Cheers

@olingern
Copy link
Contributor

olingern commented Oct 23, 2016

Yikes. I come to realize a little too late this is not a small undertaking for someone green to the codebase 😮

I'll hold off until I get some more input, but I think the flag could be a good start. The setting would need to be persisted on the user's machine, but it's not necessarily something that should be committed - as not every person's dev environment is the same.

Perhaps, the installer could generate a .yarnsettings file that is ignored. It could be written to if the flag is passed in, and read from if present.

@iredchuk
Copy link
Contributor

iredchuk commented Oct 24, 2016

I believe, if we infer the line endings from the existing file, and use OS line endings if it doesn't exist yet, should cover most of the cases, so introducing a command-line flag seems to make things unnecessary complicated. Please review the PR above.

@olingern
Copy link
Contributor

@iredchuk This would not allow for a configuration for Windows user who wants LF line endings or a unix user who wants CR.

@iredchuk
Copy link
Contributor

@olingern If git is configured to checkout with LF on Windows you would only need to change the file once per repository. Once it's checked in, it will always have LF and yarn will keep it LF by inferring the line endings.

@bel7aG

This comment was marked as spam.

@yarnpkg yarnpkg locked as resolved and limited conversation to collaborators May 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

9 participants