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

2.0.0-beta.1 now warns about files in node_modules #4931

Closed
mantoni opened this Issue Jan 13, 2016 · 19 comments

Comments

Projects
None yet
8 participants
@mantoni
Copy link

mantoni commented Jan 13, 2016

When I ugrade eslint from 2.0.0-alpha-2 to 2.0.0-beta.1, thousands of these warnings show up:

~/projects/my-project/node_modules/ws/index.js
  0:0  warning  File ignored because of a matching ignore pattern. Use --no-ignore to override

I'm running eslint with eslint "**/*.js" and without any ignores. The documentation states that node_modules are ignored by default, which is what I want.

From skimming through the commits, I guess this was introduced in #3948 but I don't understand enough about the internals to look into this myself.

Thanks for helping out.

@eslintbot

This comment has been minimized.

Copy link

eslintbot commented Jan 13, 2016

@mantoni Thanks for the issue! If you're reporting a bug, please be sure to include:

  1. The version of ESLint you are using (run eslint -v)
  2. What you did (the source code and ESLint configuration)
  3. The actual ESLint output complete with numbers
  4. What you expected to happen instead

Requesting a new rule? Please see Proposing a New Rule for instructions.

@eslintbot eslintbot added the triage label Jan 13, 2016

@ilyavolodin ilyavolodin added bug core evaluating and removed triage labels Jan 13, 2016

@gyandeeps

This comment has been minimized.

Copy link
Member

gyandeeps commented Jan 13, 2016

I also saw this happening and it happens only in above case and it doesn't happen if I add node_modules to my .eslintignore file. But I think the behavior should be consistent. Right?

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Jan 13, 2016

We should be ignoring node_modules and bower_components by default.

@nzakas nzakas added accepted and removed evaluating labels Jan 13, 2016

@gyandeeps

This comment has been minimized.

Copy link
Member

gyandeeps commented Jan 13, 2016

Interesting, there is an undocumented option which would turn off the warnings for default ignore patterns. its called warnIgnoredByDefault.

Here is the option: https://github.com/eslint/eslint/blob/master/lib/cli-engine.js#L100

Proposal:
Remove this option and by default not print warnings for default ignored files (node_modules and bower_components).

If everyone agree on this then I can work on this.
CC: @eslint/eslint-team

@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented Jan 13, 2016

Agree

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Jan 13, 2016

Agree on removing the option.

Do we currently print warning messages for Node_modules?

@gyandeeps

This comment has been minimized.

Copy link
Member

gyandeeps commented Jan 13, 2016

With < 2.0, we never printed messages for default ignores.
But with >= 2.0 we are printing ignore messages for default ignore.

With fix to this issue, we will negate the 2nd statement above and we would start behaving as we do for <2.0.
@nzakas Did i answer your question?

@btmills

This comment has been minimized.

Copy link
Member

btmills commented Jan 13, 2016

The glob (**/*.js) makes this different than the typical default ignore case. Even in v1.10.3, we would warn for node_modules when they're explicitly included by the glob. I think this is the same behavior as v1.

@btmills

This comment has been minimized.

Copy link
Member

btmills commented Jan 13, 2016

@mantoni if you run beta-1 with eslint ., do these warnings show up then too?

gyandeeps added a commit that referenced this issue Jan 13, 2016

@gyandeeps

This comment has been minimized.

Copy link
Member

gyandeeps commented Jan 13, 2016

@btmills Warnings show up when I run eslint . with beta-1 but they dont show in 1.10.3

@btmills

This comment has been minimized.

Copy link
Member

btmills commented Jan 13, 2016

@gyandeeps got it. So intended behavior is to warn with eslint **/*.js, but not with eslint .. Is that correct?

@gyandeeps

This comment has been minimized.

Copy link
Member

gyandeeps commented Jan 13, 2016

Even if run eslint 1.10 with eslint **/*.js it doesn't warn. Well nothing qualifies for me even my regular files are not getting linted.

@btmills

This comment has been minimized.

Copy link
Member

btmills commented Jan 13, 2016

Further debugging indicates my shell (zsh) was expanding the glob before passing to eslint, which is why I was getting warnings. Bash doesn't warn, even with the glob.

@gyandeeps

This comment has been minimized.

Copy link
Member

gyandeeps commented Jan 13, 2016

k as I am using bash.

@btmills

This comment has been minimized.

Copy link
Member

btmills commented Jan 13, 2016

Testing with bash, I see what everyone else here is seeing. Chalking this one up to differences in my system, carry on!

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Jan 13, 2016

We should definitely keep the < 2.0 behavior. Thanks for flagging this @gyandeeps

@nzakas nzakas closed this in b3de8f7 Jan 13, 2016

nzakas added a commit that referenced this issue Jan 13, 2016

Merge pull request #4939 from eslint/issue4931
Fix: Do not show ignore messages for default ignored files (fixes #4931)
@amb26

This comment has been minimized.

Copy link

amb26 commented May 25, 2016

This fix has done the opposite of what we want. There is now a warning for ignored default files which is impossible to turn off, since the option which disabled it has been removed. Look at the following code in glob-util.js:
https://github.com/eslint/eslint/blob/master/lib/util/glob-util.js#L165
We've just verified that the file passes "-f", which implies that the following check "-d" will fail, and so the 2nd argument to "addFile" will always be true. See the report from ember-cli-eslint, which matches the behaviour I'm seeing.

@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented May 25, 2016

Hmm.. that's actually a good point. That second attribute doesn't change. Strange. Can you link a ember-cli-eslint issue?

@IanVS

This comment has been minimized.

Copy link
Member

IanVS commented May 26, 2016

I believe the issue is ember-cli/ember-cli-eslint#63, but it's unclear to me whether it's a problem in ESLint or it is a matter of Atom's linter-eslint not using the correct eslint executable. There's a difficulty in that ember-cli-eslint seems to package its own version of eslint, which atom won't pick up on in npm@2 because it is a nested dependency (ember-cli/ember-cli-eslint#48).

As for the -d check, I wondered about that as well. Not sure why it's there, but it seems it could be replaced with true for clarity.

@eslint eslint bot locked and limited conversation to collaborators Feb 6, 2018

@eslint eslint bot added the archived due to age label Feb 6, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.