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

build: added ignore path for eslint #16010

Closed
wants to merge 1 commit into from
Closed

build: added ignore path for eslint #16010

wants to merge 1 commit into from

Conversation

chrisbudy
Copy link
Contributor

@chrisbudy chrisbudy commented Oct 6, 2017

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

build

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. windows Issues and PRs related to the Windows platform. labels Oct 6, 2017
@Trott
Copy link
Member

Trott commented Oct 7, 2017

.eslintignore is the default, so this change should be a no-op. I think the problem we saw at Code + Learn probably had to do with a problem in the .eslintignore itself?

@Trott Trott added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Oct 7, 2017
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Doesn’t seem to hurt anybody, and if it helps someone, sure

@Trott
Copy link
Member

Trott commented Oct 7, 2017

Can someone with a Windows machine (preferably @chrisbudy!) do this to confirm that this fixes anything or else confirm that it has no effect?

  1. Create a file called test/tmp/test.js. Make the contents nothing but a garbage string like Fhwqhgads.
  2. Run vcbuild lint-js. Does it show errors? If so, we have a bug. If not, something weird was up at Code + Learn.
  3. Now make the change in this PR. Run vcbuild lint-js. Does it show errors?

If the second step shows errors and the third step does not, then this PR should land. If any other combination occurs, then this PR should not land.

@refack
Copy link
Contributor

refack commented Oct 7, 2017

BTW a better command is vcbuild noprojgen nobuild lint-js

@refack
Copy link
Contributor

refack commented Oct 7, 2017

I can't repro.
Also, trying to force check the canary file gives:

D:\code\node-github-desktop$ node tools/eslint/bin/eslint.js --rulesdir=tools/eslint-rules test\tmp\test.js

D:\code\node-github-desktop\test\tmp\test.js
  0:0  warning  File ignored because of a matching ignore pattern. Use "--no-ignore" to override

✖ 1 problem (0 errors, 1 warning)

I'd also be happy to hear from @chrisbudy that this positively solves the issue.

@refack refack self-assigned this Oct 7, 2017
@refack
Copy link
Contributor

refack commented Oct 7, 2017

@chrisbudy maybe you have a global .eslintrc with a no-ignore rule?

@chrisbudy
Copy link
Contributor Author

@Trott I assumed it should default as well, but I was getting build errors on a tmp file (event though excluded in the .eslintignore file) at the code and learn, and adding the line I did stopped the build errors from happening. But I will test it again today.

Thanks for the feedback @refack and @Trott.

@BridgeAR
Copy link
Member

@chrisbudy did you get the chance to test this again?

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Someone else tested this and unfortunately this doesn't solve the problem. See details here: #16278 (comment)

@Trott
Copy link
Member

Trott commented Oct 21, 2017

I'm going to close this one but feel free to re-open or comment if you think this should stay open. The solution is likely a bug-fix in ESLint. Alternatively, we could add a common.refreshTmpDir() to the end of the test that creates the directory with multi-byte characters, but I'd prefer this be run up to ESLint instead, TBH. Will need some Windows folks willing to dig a bit, though. That's not me, unfortunately.

@Trott Trott closed this Oct 21, 2017
@refack refack mentioned this pull request Oct 21, 2017
2 tasks
refack added a commit to refack/node that referenced this pull request Oct 26, 2017
Makes eslint exclude directories without enumerating their content

PR-URL: nodejs#16372
Refs: nodejs#16010
Refs: nodejs#16278
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
refack added a commit to refack/node that referenced this pull request Oct 26, 2017
`.tmp` prefix allows easier exclusion

PR-URL: nodejs#16372
Refs: nodejs#16010
Refs: nodejs#16278
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
refack added a commit to refack/node that referenced this pull request Oct 26, 2017
also undocument the `vcbuild.bat` command since it's broken
and seems to only be relevant to release builds

PR-URL: nodejs#16372
Refs: nodejs#16010
Refs: nodejs#16278
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
gibfahn pushed a commit that referenced this pull request Oct 30, 2017
Makes eslint exclude directories without enumerating their content

PR-URL: #16372
Refs: #16010
Refs: #16278
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
gibfahn pushed a commit that referenced this pull request Oct 30, 2017
`.tmp` prefix allows easier exclusion

PR-URL: #16372
Refs: #16010
Refs: #16278
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
gibfahn pushed a commit that referenced this pull request Oct 30, 2017
also undocument the `vcbuild.bat` command since it's broken
and seems to only be relevant to release builds

PR-URL: #16372
Refs: #16010
Refs: #16278
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
gibfahn pushed a commit that referenced this pull request Oct 30, 2017
Makes eslint exclude directories without enumerating their content

PR-URL: #16372
Refs: #16010
Refs: #16278
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
gibfahn pushed a commit that referenced this pull request Oct 30, 2017
`.tmp` prefix allows easier exclusion

PR-URL: #16372
Refs: #16010
Refs: #16278
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
gibfahn pushed a commit that referenced this pull request Oct 30, 2017
also undocument the `vcbuild.bat` command since it's broken
and seems to only be relevant to release builds

PR-URL: #16372
Refs: #16010
Refs: #16278
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
gibfahn pushed a commit that referenced this pull request Oct 31, 2017
Makes eslint exclude directories without enumerating their content

PR-URL: #16372
Refs: #16010
Refs: #16278
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
gibfahn pushed a commit that referenced this pull request Oct 31, 2017
`.tmp` prefix allows easier exclusion

PR-URL: #16372
Refs: #16010
Refs: #16278
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
gibfahn pushed a commit that referenced this pull request Oct 31, 2017
also undocument the `vcbuild.bat` command since it's broken
and seems to only be relevant to release builds

PR-URL: #16372
Refs: #16010
Refs: #16278
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
Makes eslint exclude directories without enumerating their content

PR-URL: nodejs/node#16372
Refs: nodejs/node#16010
Refs: nodejs/node#16278
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
`.tmp` prefix allows easier exclusion

PR-URL: nodejs/node#16372
Refs: nodejs/node#16010
Refs: nodejs/node#16278
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
also undocument the `vcbuild.bat` command since it's broken
and seems to only be relevant to release builds

PR-URL: nodejs/node#16372
Refs: nodejs/node#16010
Refs: nodejs/node#16278
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
Makes eslint exclude directories without enumerating their content

PR-URL: nodejs/node#16372
Refs: nodejs/node#16010
Refs: nodejs/node#16278
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
`.tmp` prefix allows easier exclusion

PR-URL: nodejs/node#16372
Refs: nodejs/node#16010
Refs: nodejs/node#16278
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
also undocument the `vcbuild.bat` command since it's broken
and seems to only be relevant to release builds

PR-URL: nodejs/node#16372
Refs: nodejs/node#16010
Refs: nodejs/node#16278
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
Makes eslint exclude directories without enumerating their content

PR-URL: nodejs/node#16372
Refs: nodejs/node#16010
Refs: nodejs/node#16278
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
`.tmp` prefix allows easier exclusion

PR-URL: nodejs/node#16372
Refs: nodejs/node#16010
Refs: nodejs/node#16278
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
also undocument the `vcbuild.bat` command since it's broken
and seems to only be relevant to release builds

PR-URL: nodejs/node#16372
Refs: nodejs/node#16010
Refs: nodejs/node#16278
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@refack refack removed their assignment Oct 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants