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

--no-ignore should not un-ignore node_modules #5547

Closed
IanVS opened this issue Mar 11, 2016 · 12 comments
Closed

--no-ignore should not un-ignore node_modules #5547

IanVS opened this issue Mar 11, 2016 · 12 comments
Assignees
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly core Relates to ESLint's core APIs and features

Comments

@IanVS
Copy link
Member

IanVS commented Mar 11, 2016

What version of ESLint are you using?
Master

What configuration and parser (Espree, Babel-ESLint, etc.) are you using?
Espree

What did you do? Please include the actual source code causing the issue.
Ran eslint with --no-ignore flag

What did you expect to happen?
Ignore settings from .eslintignore to be disregarded, as indicated in docs.

What actually happened? Please include the actual, raw output from ESLint.
ESLint linted all files within node_modules.

Despite having tests to assert that --no-ignore will cause node_modules to be linted, I think this is not the behavior we expect or want.

The --no-ignore docs say:

--no-ignore
Disables excluding of files from .eslintignore and --ignore-path files.

They don't say anything about also disabling default ignores.

After #5410 is fixed, it will be possible to lint within node_modules by using a negation pattern. (e.g. !/node_modules/package-to-lint/), at which time I think it would make sense to keep --no-ignore from linting /node_modules/ and /bower_components/.

Does the team agree?

@IanVS IanVS added bug ESLint is working incorrectly core Relates to ESLint's core APIs and features evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Mar 11, 2016
@ilyavolodin
Copy link
Member

Agree.

@alberto
Copy link
Member

alberto commented Mar 11, 2016

Wouldn't this be a breaking change?

@IanVS
Copy link
Member Author

IanVS commented Mar 11, 2016

Personally I see it as a bug fix, since the current behavior is not as
documented or expected.

On Fri, Mar 11, 2016, 12:37 alberto notifications@github.com wrote:

Wouldn't this be a breaking change?


Reply to this email directly or view it on GitHub
#5547 (comment).

@alberto
Copy link
Member

alberto commented Mar 22, 2016

I just realized that if --no-ignore ignores .eslintignore files, it won't be possible to uningnore anything at the same time 😅

@platinumazure
Copy link
Member

@alberto Is that really so, given one could specify ignore patterns in the CLI using --ignore-pattern?

@nzakas nzakas added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Mar 22, 2016
@nzakas
Copy link
Member

nzakas commented Mar 22, 2016

--no-ignore is mostly used for testing purposes (at least, that's why it was added, IIRC). I agree, this seems like a bug that was leftover after the ignore file refactoring.

@alberto
Copy link
Member

alberto commented Mar 22, 2016

@platinumazure not very intuitive, but 👍 given the context.

@nzakas nzakas closed this as completed in 66db38d Mar 25, 2016
nzakas added a commit that referenced this issue Mar 25, 2016
Fix: `--no-ignore` should not un-ignore default ignores (fixes #5547)
@IanVS
Copy link
Member Author

IanVS commented Mar 28, 2016

Unfortunately it looks like #5648 did not fully fix this issue. It addressed it inside of ignored-paths.js, but we also do some checking for options.ignore in cli-engine.js in isPathIgnored() and executeOnText().

Testing manually on the cli reveals that --no-ignore still causes node_modules to be linted as before.

@IanVS IanVS reopened this Mar 28, 2016
@nzakas
Copy link
Member

nzakas commented Mar 28, 2016

So this would only affect people using the API?

@IanVS
Copy link
Member Author

IanVS commented Mar 28, 2016

The fix only was made in ignored-paths, which caused its own tests to pass. The ESLint API and CLI are still broken.

@alberto
Copy link
Member

alberto commented Mar 28, 2016

Ooops, sorry. I would swear I had also manually tested it... :(

@IanVS
Copy link
Member Author

IanVS commented Apr 11, 2016

Working on this.

@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly core Relates to ESLint's core APIs and features
Projects
None yet
Development

No branches or pull requests

5 participants