You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
After e77e935 changes made by @Trott, jslint error fails on windows machine because linebreak-style rule expects linebreaks to be LF but on windows they are CRLF and this fails the linebreak-style rule for all *.js files.
It sounds like you have core.autocrlf set to true in your git config. Try setting git config core.eol lf and disable core.autocrlf - and make sure your editor uses UNIX line endings, of course.
Thanks @bnoordhuis , @silverwind . However as seen in git's help message, I am using the recommended setting core.autocrlf=true on windows machine for cross platform project. How about passing it programmatically inside tools\jslint.js file using return value from process.platform?
core.autocrlf=true has been discussed in #2494 , there was no consensus but we kept it supported at the time.
Windows CI servers use core.autocrlf=true (no warning in #6685 because linting is not run on Windows).
I believe most users who compile on Windows have core.autocrlf=true and I would not welcome a platform specific recommendation to change it. Most importantly, if we want to support end users running node on CRLF javascript files, we should keep testing this way and keep core.autocrlf=true fully supported. Thus, I would welcome a change to make vcbuild jslint work again.
Most importantly, if we want to support end users running node on CRLF javascript files, we should keep testing this way and keep core.autocrlf=true fully supported.
I don't understand this comment. This issue is about our test suite, not node-on-windows in general.
The main purpose of the lint rule is to let contributors know if they are about to submit code (in a PR) that does not conform to the existing code base.
If the typical user setup on Windows is to have git convert line endings to CRLF on check-out and back to LF on check-in, then the lint rule should probably be removed.
If that's an atypical setup or a setup we want to discourage for other reasons, then the lint rule should remain as-is.
I don't think there's much to be gained by adding complexity and having the rule check for different line endings on different platforms. That probably would not solve any real issues, and might create a few new ones.
I agree with @Trott . Instead of complicating things here, we should remove linebreak-style from jslint or reduce its severity. As I mentioned earlier, git does recommend having core.autocrlf=true for windows users. It is very unlikely that developer working on windows would use IDE with unix ending.
All files in the repository should have UNIX line endings. Contributors are expected to follow the project's conventions and the new lint rule helps with that. I don't see how that is controversial.
It is very unlikely that developer working on windows would use IDE with unix ending.
That seems like a very weak argument. You're either saying that Windows IDEs are defective or that Windows developers are too lazy and/or clueless to configure their editor. Both seem implausible.
Line endings on Windows are different than on Unix. As awkward as it may seem, it's a platform difference that we simply have to live with.
IMO, core.autocrlf=true is the correct default for Windows users. It allows us to have text files with portable line endings. If we absolutely want the Unix line ending, we can mark the file as binary.
Without this lint rule, how would you catch CRLFs when someone has core.autocrlf disabled and checks in changes with Windows line endings?
It's not a rhetorical question. Windows line endings have slipped in several times over the years. They broke the build at least once (25a5e90) although that wasn't in the test suite.
The only solution I can think of is a new CI lint check but that seems like a lot of effort when people just have to run git config core.eol lf locally once.
Moving away from CRLF line endings, like disabling core.autocrlf, is the wrong answer to this issue. If we did something like that, all js test files would have LF line endings, and possible bugs affecting only CRLF js files would not be caught.
We should check for the correct line endings in the linter because the repository should contain only LF line endings, as @bnoordhuis mentioned above. @Trott, I think the rule should be used everywhere, not only in CI. I am working on a PR to fix this.
possible bugs affecting only CRLF js files would not be caught
If you have concerns that node doesn't handle CRLF correctly in all cases, we can add regression tests and whitelist them. That would be a good addition regardless of the outcome of this issue.
@bnoordhuis I believe that CRLF is handled correctly now because we are testing it in CI. We should keep testing it in general (all files on Windows machines) and not only some specific test, because CRLF is the correct line ending on Windows.
Activity
bnoordhuis commentedon May 21, 2016
It sounds like you have
core.autocrlfset to true in your git config. Try settinggit config core.eol lfand disablecore.autocrlf- and make sure your editor uses UNIX line endings, of course.silverwind commentedon May 21, 2016
Or alternatively, get git to behave with the third option here (globally):
kunalspathak commentedon May 22, 2016
Thanks @bnoordhuis , @silverwind . However as seen in git's help message, I am using the recommended setting
core.autocrlf=trueon windows machine for cross platform project. How about passing it programmatically insidetools\jslint.jsfile using return value fromprocess.platform?tool: modified eslint rule for line-breaks
bnoordhuis commentedon May 23, 2016
You can configure
core.autocrlfon a per-project setting. I doubt we'd accept patches to work around local configuration issues.joaocgreis commentedon May 23, 2016
core.autocrlf=truehas been discussed in #2494 , there was no consensus but we kept it supported at the time.Windows CI servers use
core.autocrlf=true(no warning in #6685 because linting is not run on Windows).I believe most users who compile on Windows have
core.autocrlf=trueand I would not welcome a platform specific recommendation to change it. Most importantly, if we want to support end users running node onCRLFjavascript files, we should keep testing this way and keepcore.autocrlf=truefully supported. Thus, I would welcome a change to makevcbuild jslintwork again.cc @nodejs/testing
bnoordhuis commentedon May 23, 2016
I don't understand this comment. This issue is about our test suite, not node-on-windows in general.
Trott commentedon May 23, 2016
The main purpose of the lint rule is to let contributors know if they are about to submit code (in a PR) that does not conform to the existing code base.
If the typical user setup on Windows is to have git convert line endings to CRLF on check-out and back to LF on check-in, then the lint rule should probably be removed.
If that's an atypical setup or a setup we want to discourage for other reasons, then the lint rule should remain as-is.
I don't think there's much to be gained by adding complexity and having the rule check for different line endings on different platforms. That probably would not solve any real issues, and might create a few new ones.
kunalspathak commentedon May 24, 2016
@bnoordhuis , I think @joaocgreis meant node contributors working on windows platform.
I agree with @Trott . Instead of complicating things here, we should remove
linebreak-stylefrom jslint or reduce its severity. As I mentioned earlier, git does recommend havingcore.autocrlf=truefor windows users. It is very unlikely that developer working on windows would use IDE with unix ending.bnoordhuis commentedon May 24, 2016
All files in the repository should have UNIX line endings. Contributors are expected to follow the project's conventions and the new lint rule helps with that. I don't see how that is controversial.
That seems like a very weak argument. You're either saying that Windows IDEs are defective or that Windows developers are too lazy and/or clueless to configure their editor. Both seem implausible.
orangemocha commentedon May 24, 2016
Line endings on Windows are different than on Unix. As awkward as it may seem, it's a platform difference that we simply have to live with.
IMO,
core.autocrlf=trueis the correct default for Windows users. It allows us to have text files with portable line endings. If we absolutely want the Unix line ending, we can mark the file as binary.bnoordhuis commentedon May 24, 2016
Without this lint rule, how would you catch CRLFs when someone has
core.autocrlfdisabled and checks in changes with Windows line endings?It's not a rhetorical question. Windows line endings have slipped in several times over the years. They broke the build at least once (25a5e90) although that wasn't in the test suite.
The only solution I can think of is a new CI lint check but that seems like a lot of effort when people just have to run
git config core.eol lflocally once.Trott commentedon May 24, 2016
I'd be OK with a separate ESLint config file for CI only that included just two things:
extendsconfiguration indicating the actual ESLint config file that we use now (EDIT: with the EOL rule removed from that file, of course)tool: modified eslint rule for line-breaks
joaocgreis commentedon May 26, 2016
Moving away from CRLF line endings, like disabling core.autocrlf, is the wrong answer to this issue. If we did something like that, all js test files would have LF line endings, and possible bugs affecting only CRLF js files would not be caught.
We should check for the correct line endings in the linter because the repository should contain only LF line endings, as @bnoordhuis mentioned above. @Trott, I think the rule should be used everywhere, not only in CI. I am working on a PR to fix this.
Trott commentedon May 26, 2016
That describes the current situation.
bnoordhuis commentedon May 26, 2016
If you have concerns that node doesn't handle CRLF correctly in all cases, we can add regression tests and whitelist them. That would be a good addition regardless of the outcome of this issue.
joaocgreis commentedon May 30, 2016
@bnoordhuis I believe that CRLF is handled correctly now because we are testing it in CI. We should keep testing it in general (all files on Windows machines) and not only some specific test, because CRLF is the correct line ending on Windows.
joaocgreis commentedon May 30, 2016
PR: #7019
win,tools: ignore linting for line breaks
win,tools: ignore linting for line breaks
win,tools: ignore linting for line breaks
win,tools: ignore linting for line breaks