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 CRLF EOL for Window-specific file formats and add diffs #129

Merged
merged 1 commit into from
Mar 23, 2021
Merged

Add CRLF EOL for Window-specific file formats and add diffs #129

merged 1 commit into from
Mar 23, 2021

Conversation

david-fong
Copy link
Contributor

@david-fong david-fong commented Mar 22, 2021

Note on package.json:

Therefore, the line specifying package.json to use eol=lf is for pnpm and not needed for npm and yarn.

@Richienb Richienb changed the title eol for ps1 & web meta, and markdown diff headings Add CRLF EOL for Window-specific file formats and add diifs Mar 23, 2021
@Richienb Richienb changed the title Add CRLF EOL for Window-specific file formats and add diifs Add CRLF EOL for Window-specific file formats and add diffs Mar 23, 2021
@Richienb Richienb merged commit c9e0391 into gitattributes:master Mar 23, 2021
@david-fong
Copy link
Contributor Author

@Richienb I learned more about how npm and yarn handle line endings for package.json. I updated the description. Can you take a look and weigh in on whether you think this template should simply go with lf for everything or alternatively add a comment that lf should be specified if using pnpm?

Also, since this PR for npm npm/npm#19904 got merged in 2018, I think it may be okay to remove the eol specifier I added for package-lock.json. I just tested it: npm creates a package-lock.json with lf by default, but if you switch it to crlf (like it would be fore someone who checks out the repo) and then perform an action that modifies the lockfile, npm goes along with the crlf. What do you think?

@Richienb
Copy link
Member

Unless absolutely required, we shouldn't restrict files to specific line endings.

@devinrhode2
Copy link
Contributor

Why not just keep it simple and consistent? package.json needs to be converted to LF, so why not also convert package-lock.json to LF?

@devinrhode2
Copy link
Contributor

Until about March 18th 2018 npm would normalize to LF, so to be compatible with older versions of npm, normalizing to LF would be better IMO.

I'd say, if the be-all end-all to .gitattributes files causes files to be normalized to LF, then, I'll trust that project has made the right decision, and commit this to the repo.

If we allow LF or CRLF, if someone is working on a project that may have already normalized to LF, and is using windows, they could end up in a situation where they are accidentally changing EOL to CRLF

I sort of look to this project to just handle all CRLF/LF issues.. I am happy to see EOL normalized almost completely to LF, but some people more unfamiliar with EOL changes in git may be more confused.

..Now, if we have to convert package.json to LF for consistency with package managers, why not also be compatible with npm@5 and convert package-lock.json to LF?

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.

3 participants