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

linebreak-style makes it impossible for Windows users to contribute #1224

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

anaisbetts
Copy link

Any project that currently uses eslint-config-airbnb checked out on Windows results in thousands of linter errors since the line endings will be CRLF, making it impossible to contribute to these projects as a Windows user without nerfing the linter entirely.

A better way to enforce this is not via a linter, but via a .gitattributes file (https://help.github.com/articles/dealing-with-line-endings). When you do this, Git will ensure the repo internally is LF, but on Win32 will check out files as CRLF (this is a builtin case of a clean/smudge filter)

Refs vercel/hyper#1230, xojs/eslint-config-xo#35

Any project that currently uses eslint-config-airbnb checked out on Windows results in thousands of linter errors since the line endings will be CRLF, making it impossible to contribute to these projects as a Windows user without nerfing the linter entirely.

A better way to enforce this is not via a linter, but via a .gitattributes file (https://help.github.com/articles/dealing-with-line-endings). When you do this, Git will ensure the repo internally is LF, but on Win32 will check out files as CRLF (this is a builtin case of a clean/smudge filter)

Refs vercel/hyper#1230, xojs/eslint-config-xo#35
@SimenB
Copy link
Contributor

SimenB commented Dec 29, 2016

#1089 (comment)

Also, it should be set to off, not removed.

@ljharb
Copy link
Collaborator

ljharb commented Dec 29, 2016

Indeed, #1089 (comment) is the official response - all users should always only use LF, including on Windows.

@ljharb ljharb closed this Dec 29, 2016
@anaisbetts
Copy link
Author

anaisbetts commented Dec 29, 2016

@ljharb This is not how Git works on Windows, can you please read https://help.github.com/articles/dealing-with-line-endings/#platform-windows and http://adaptivepatchwork.com/2012/03/01/mind-the-end-of-your-line? What you're asking is imposing a huge burden on Windows users. To contribute to any AirBnB repo, users will have to:

  1. Edit their system-wide Git settings to set autocrlf=false
  2. Check out the repo (remember, it can't be done after-the-fact, because all the files will be checked out as CRLF)
  3. Remember to set autocrlf=true back before working with any other repo on their system.
  4. Continue to remember to toggle this setting back-and-forth constantly

You're entirely correct that editors on Windows will be perfectly happy to work in LF, but this isn't an editor problem - this is a fundamental conflict with how Git works on Windows. I don't know why ESLint added this feature but imho I agree with @SimenB, the only thing ESLint should be checking for is inconsistent line endings.

If I add this feature to ESLint, will you consider using it? Line Ending Purity isn't worth throwing 50%+ of node.js users under the bus.

@anaisbetts
Copy link
Author

/cc @haacked @shiftkey from GitHub who probably have Thoughts on this issue

@ljharb
Copy link
Collaborator

ljharb commented Dec 29, 2016

Certainly a consistent option would make sense to me, and I'd vastly prefer a discussion about changing "LF only" to "consistent, LF preferred" over the current one of "LF only" vs "off". (however, I'd want a way where on non-Windows systems, CRLF was strictly forbidden - there's no reason for a CRLF character to exist outside of a Windows machine)

What I'm suggesting is that every repo on their system, and that they work with, should be using LF, and not CRLF. I agree that it's highly inconvenient that they'd have to re-clone all their repos after setting their systemwide Git settings to autocrlf=false - but I'm then suggesting that they'd never set it back to true, and that any Windows projects they work with should be using only LF as well.

What are the arguments (besides "legacy", which carries weight but isn't the best reason to make choices) why any project should ever use CRLF?

@anaisbetts
Copy link
Author

What are the arguments (besides "legacy", which carries weight but isn't the best reason to make choices) why any project should ever use CRLF?

Whether LF or CRLF is philosophically right or not doesn't actually matter, what matters is that this setting system-wide is both non-default (and relatively rare tbh), and will definitely corrupt repos that have CRLF encoded in the repo (also non-default, but far more common, from the last time I ran the statistics on the top 250 C# repos, as part of my work on the Windows GitHub client).

So in the very best case scenario, you're asking every Windows user to reclone every repo on their system (and to understand all of these nuances around line endings), and in the worst case, you're encouraging users to set a setting that will corrupt every PR they submit to certain repos.

In the meantime, I'm desperately trying to get Windows users to contribute to node.js projects - all I hear from JS maintainers are "Ugh, Windows people never contribute, they just complain". Help me fix that by getting this obstacle out of the way!

@ljharb
Copy link
Collaborator

ljharb commented Dec 29, 2016

If the "consistent" option were available, I'd be willing to conditionally set it to "unix" or "consistent, unix preferred" based on process.platform === 'win32' - that way at least non-Windows users (and almost every CI system) would require LF, but windows users would have a more relaxed setting.

Do you think that would address the line endings footguns on Windows you're describing, without completely sacrificing the desired guarantees?

@anaisbetts
Copy link
Author

That would do it, yeah - lemme work on an ESLint PR today

Copy link
Collaborator

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reopening this PR - pending an eslint rule change.

@anaisbetts
Copy link
Author

@ljharb eslint/eslint#7823 should get us what we need here, then we can write something like:

"linebreak-style": ["error", process.platform === "win32" ? "consistent" : "unix"]

anaisbetts added a commit to electron/forge that referenced this pull request Jan 2, 2017
See airbnb/javascript#1224,
xojs/eslint-config-xo#35, and
eslint/eslint#7823 for the sad details.

This doesn't completely solve the greater issue, but should at least
help a little bit to ensure new Electron projects can be used on
Windows
MarshallOfSound pushed a commit to electron/forge that referenced this pull request Jan 2, 2017
See airbnb/javascript#1224,
xojs/eslint-config-xo#35, and
eslint/eslint#7823 for the sad details.

This doesn't completely solve the greater issue, but should at least
help a little bit to ensure new Electron projects can be used on
Windows
@anaisbetts
Copy link
Author

I'm not interested in pursuing this further. Thanks all.

@anaisbetts anaisbetts closed this Jan 25, 2017
@ljharb
Copy link
Collaborator

ljharb commented Jan 25, 2017

I'm sorry to hear that. I'm going to reopen it, however, since it's still pending an eslint rule change.

Feel free to unsubscribe from the thread if you're not interested in further participation.

@ljharb ljharb reopened this Jan 25, 2017
@jacobq
Copy link

jacobq commented Mar 22, 2017

I'm not interested in having / starting a flame war, but to anyone who thinks there is no use case for allowing / supporting different EOL encodings, I can tell you that as embedded software engineer I have to work with it all the time as many devices (typically closed-source / black boxes) require it as part of their interface / API (e.g. data sent via USB/serial port or TCP). Just because you have the privilege of working in an environment where you can have control and can make things the way you want does not mean that all developers can or should. Having consistent code standards is great, but if it's taken too far the cost/benefit ratio can explode due to lost productivity dealing with minutia.

@ljharb
Copy link
Collaborator

ljharb commented Mar 22, 2017

@jacobq Unless you do your typing on those machines, I'm not sure I see the problem - you can always transform your code before you load it onto a device.

@daxelrod
Copy link
Contributor

daxelrod commented Jun 1, 2017

The sanest way I've found to configure projects so that this rule is followed on Windows is:

A .gitattributes in the project with

* text=auto eol=lf

which will represent files as LF in the object database and write them with LF on checkout, and

A .editorconfig in the project with

end_of_line = lf

to cause many editors to initially create files with LF endings.

@ljharb
Copy link
Collaborator

ljharb commented Jun 13, 2017

An editorconfig is a great idea, as soon as you can hook up a tool that can enforce that the editorconfig doesn't drift - iow, that it matches (or is compatible with) your eslint config, and all your files comply with it. I'm not aware of a reliable one.

@iamakulov
Copy link

iamakulov commented Nov 25, 2017

Folks, what’s the status of this? I’d love to see this merged (do tell if I can help in any way!).

A lot of projects use the AirBnB config, and it’s very annoying to constantly add 'linebreak-style': 'off' to their .eslintrc (and be careful to not commit this change) when I try to contribute to them.

@ljharb
Copy link
Collaborator

ljharb commented Nov 25, 2017

@iamakulov at the moment, it's blocked by eslint's refusal to allow eslint/eslint#7823 to be merged.

@iamakulov
Copy link

@ljharb Thanks!

While ESLint is blocking this, what if we solve this issue with

"linebreak-style": ["error", process.platform === "win32" ? "windows" : "unix"]

?

My thinking is as follows:

  1. By default, Git uses CRLF for Windows files and LF for Unix files. That code ↑ matches this default behavior.
  2. The default behavior could be changed either by i) adding a .gitattributes file or ii) changing the core.autocrlf option globally.
    1. The .gitattributes is added by a maintainer. The maintainer would know the effect of the * eol=lf statement and would likely remember to change the linebreak-style rule accordingly. Even if they won’t, Windows users would report a new ESLint error, and the maintainer would likely remember that they added .gitattributes before that.
    2. The core.autocrlf option is changed by a specific user. The user would know what they do, so if they configure Git to check out in LF, they’d likely understand why ESLint tells them line endings should be CRLF. In this case, they might either disable this setting or submit a PR with the .gitattributes files.

This would keep the majority of people that use the default settings happy. The power users receiving the error should understand why it’s happening.

@ljharb
Copy link
Collaborator

ljharb commented Nov 26, 2017

@iamakulov the concern there is then a repo might not add the .gitattributes file in the first place, and conflicts could arise between windows and linux developers using the airbnb config.

@iamakulov
Copy link

iamakulov commented Nov 27, 2017

the concern there is then a repo might not add the .gitattributes file in the first place, and conflicts could arise between windows and linux developers using the airbnb config.

@ljharb Could you elaborate on that please? AFAIK, the repo (not the local checked-out code, but the commits) would always be in LF, so there should be no conflicts.

@iamakulov
Copy link

iamakulov commented Nov 27, 2017

AFAIK, the repo (not the local checked-out code, but the commits) would always be in LF

I.e.:

  • On Windows, Git uses core.autocrlf=true by default. This keeps local (checked out) files in CRLF, but converts them to LF when they are committed.
  • On UNIX, Git uses core.autocrlf=input by default. This keeps local (checked out) files in LF + makes sure the files are also committed with LF (i.e. if CRLF is encountered, it’s converted to LF).

Here’s the related piece of docs: https://git-scm.com/book/en/v2/Customizing-Git-Git-Configuration (Ctrl+F → “core.autocrlf”)

So, the repo (as in the commits) are always kept in LF by default, and developers don’t have any Git conflicts. But local files use CRLF on Windows and LF on Unix, and this is what this rule matches:

"linebreak-style": ["error", process.platform === "win32" ? "windows" : "unix"]

@ljharb
Copy link
Collaborator

ljharb commented Dec 17, 2017

@iamakulov my understanding is that the behavior you're describing only applies if windows users have set up their git properly using autocrlf; the documentation you linked doesn't say what the default is, but I'm relatively sure the motivation for eslint/eslint#7823 was that it's not the default behavior, and that once changed, you have to delete and re-clone any previously cloned repo to fix it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants