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

When running eslint from another directory and using --ignore-path, paths are not relative to .eslintignore location #6759

Closed
ratherblue opened this issue Jul 26, 2016 · 30 comments
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 breaking This change is backwards-incompatible bug ESLint is working incorrectly core Relates to ESLint's core APIs and features
Projects
Milestone

Comments

@ratherblue
Copy link

ratherblue commented Jul 26, 2016

What version of ESLint are you using?
3.1.1

What parser (default, Babel-ESLint, etc.) are you using?
default

Please show your full configuration:
Project structure:

.eslintignore
.eslintrc
bar.js
src/foo.js

.eslintrc

{
    "root": true,
    "extends": "eslint:recommended"
}

.eslintignore

src/foo.js

What did you do? Please include the actual source code causing the issue.

$ eslint foo.js --ignore-path ../.eslintignore

C:\dev\eslint-bug\src\foo.js
  1:1  error  'test' is not defined  no-undef

✖ 1 problem (1 error, 0 warnings)

What did you expect to happen?
Expected C:\dev\eslint-bug\src\foo.js to be ignored

What actually happened? Please include the actual, raw output from ESLint.
Errors were reported for C:\dev\eslint-bug\src\foo.js:

C:\dev\eslint-bug\src\foo.js
  1:1  error  'test' is not defined  no-undef

✖ 1 problem (1 error, 0 warnings)

In Configuring ESLint it says "Paths are relative to .eslintignore location or the current working directory". As a user, I would assume that if I passed in --ignore-path that it would ignore files as if I had called ESLint from that directory.

I see that there have been a lot of issues opened regarding ESLint ignore and parent directories, but after #5694, ignoring files in SublimeText with SublimeLinter-eslint has been broken. There is an open PR to fix an existing issue with loading .eslintignore in SublimeLinter-eslint (SublimeLinter/SublimeLinter-eslint#145), but it doesn't work after ESLint 2.5.0.

There is an option in the CLIEngine to specify a cwd. If my above process is "working as intended" (or perhaps I have misinterpreted the documentation), then it would be great to be able to pass in a --cwdoption for the command line so that the relative paths can be loaded correctly.

@eslintbot eslintbot added the triage An ESLint team member will look at this issue soon label Jul 26, 2016
@ratherblue ratherblue changed the title When running eslint from another directory and using --ignore-path, paths are not relative to .eslintignore location When running eslint from another directory and using --ignore-path, paths are not relative to .eslintignore location Jul 26, 2016
@alberto alberto 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 and removed triage An ESLint team member will look at this issue soon labels Jul 27, 2016
@IanVS
Copy link
Member

IanVS commented Jul 28, 2016

It seems reasonable to me to use a provided --ignore-path directory as the base dir when we convert the filename to a relative path. I've tried a possible implementation of this and it seems to work and not break any of our existing tests, would be happy to submit a PR if this is accepted by the TSC.

@alberto
Copy link
Member

alberto commented Jul 28, 2016

Changing that would be a breaking change.

@IanVS
Copy link
Member

IanVS commented Jul 28, 2016

Or would it be a bugfix? I can't think of a single reason why someone would want the behavior of the .eslintignore file to depend on cwd when linting is performed. I doubt that was intended, but maybe I'm wrong. That's something for the TSC to decide anyway. I'm tossing it on the agenda.

@IanVS IanVS added the tsc agenda This issue will be discussed by ESLint's TSC at the next meeting label Jul 28, 2016
@alberto
Copy link
Member

alberto commented Jul 30, 2016

We prefer an open discussion in the issue before putting in in the TSC agenda, so we can get feedback from everyone.

One possible use case is someone keeping the ignore file outside the project.

@mysticatea
Copy link
Member

Hmm, does git have the feature correspond to our --ignore-path for .gitignore? If yes, we should make the same behavior as that. In this case, this is a bug.

If no, we should consider proper behavior. In this case, I think too aggressive if we address this as a bug. For example, there might be people which use --ignore-path as like:

{
    "lint-src": "eslint . -c conf/src.eslintrc.json --ignore-path conf/src.eslintignore",
    "lint-test": "eslint . -c conf/test.eslintrc.json --ignore-path conf/test.eslintignore"
}

@nzakas
Copy link
Member

nzakas commented Aug 1, 2016

@IanVS please add a comment for the TSC that summarizes what you're seeking guidance on (see http://eslint.org/docs/maintainer-guide/issues.html#when-to-send-to-tsc for the format).

@IanVS IanVS removed the tsc agenda This issue will be discussed by ESLint's TSC at the next meeting label Aug 2, 2016
@IanVS
Copy link
Member

IanVS commented Aug 2, 2016

@nzakas thanks. There have been a lot of changes to our process lately and I admit I haven't kept up as much as I should have. In gitter you had said,

Core and CLI changes require TSC approval

So I thought I needed to get this on the agenda to approve its acceptance as a bug.

@IanVS
Copy link
Member

IanVS commented Aug 2, 2016

Consider this file structure:

.
└── top-level-dir
    ├── .ignore
    ├── dist
    │   └── foo.js
    └── src
        └── foo.js

And imagine the contents of .ignore is:

/foo.js

Here are a few commands that could be run, along with the results as they currently stand:

> pwd #->  top-level-dir
> eslint --ignore-path .ignore src/foo.js dist/foo.js  # -> file is linted

> cd src
> eslint --ignore-path ../.ignore foo.js # -> file is ignored

As you can see, linting the same file with the same ignore file has different outcomes depending on where the command is run. Even though the .ignore specifies the file with a leading slash, it can still match files in a nested folder. This goes directly against the way .gitignore files work. From the gitignore docs:

These patterns match relative to the location of the .gitignore file.

This is why I think this is a bug that should be fixed. The patterns specified in an eslint ignore file should also be relative to the location of the file, not relative to whatever location eslint is invoked from.

@IanVS
Copy link
Member

IanVS commented Aug 2, 2016

@alberto, @mysticatea The fix I'm proposing here would not prevent either of those use cases. The contents of the ignore files being used would just simply need to contain proper patterns/globs which match the desired files to ignore based on their relative position to the ignore file itself.

@nzakas
Copy link
Member

nzakas commented Aug 2, 2016

@IanVS you are always free to accept bugs that you have verified, regardless of their location.

@IanVS
Copy link
Member

IanVS commented Aug 2, 2016

There was some discussion whether this is actually a bug or a feature. I feel pretty strongly it's a bug, but willing to hear counterarguments.

@platinumazure
Copy link
Member

I think this might be a bug.

At my work, we run eslint multiple times and pass in directories directly, and we don't need to have an .eslintignore as a result. At one point, I tried to write an .eslintignore and reconfigure our build process to just run ESLint once. We had to use --ignore-path because our CWD was somewhere else. And files that should have been ignored were not ignored. My conjecture now is that this is because of this issue.

I do think it makes more sense (principle of least surprise) for .eslintignore globs to be relative to the .eslintignore directory. That's similar to how .gitignore works (Git won't suddenly start tracking files it shouldn't just because I happen to run git status in a project subdirectory). So I am very much 👍 for this change. I probably would have reported this issue myself, if I had been able to make the connection between why .eslintignore didn't seem to be ignoring and the fact that ESLint was run in a subdirectory.

@alberto
Copy link
Member

alberto commented Aug 3, 2016

@IanVS I think what you are proposing is very reasonable, and I am not saying it would prevent the use cases we mentioned, just that it will change the current behaviour. It will affect people relying on the current one (like the example @mysticatea showed), whose setup will stop working and they'll have to change the ignore file paths. That makes me a bit reluctant to introduce this change in a minor release.

@IanVS
Copy link
Member

IanVS commented Aug 3, 2016

@alberto, can you explain? I don't see how this effects @mysticatea's example.

@nzakas
Copy link
Member

nzakas commented Aug 3, 2016

@IanVS ah, gotcha. If you're unsure if this is a

@nzakas nzakas closed this as completed Aug 3, 2016
@nzakas nzakas reopened this Aug 3, 2016
@nzakas
Copy link
Member

nzakas commented Aug 3, 2016

Sorry, my tablet freaked out and accidentally hit the wrong button.

What I was saying: if there's a question about whether or not this is a bug and whether or not we would need a major release to address, then it should go in the TSC agenda. We'd just need a comment summarizing the issue and explaining what question(s) the TSC should answer.

@mysticatea
Copy link
Member

@IanVS in my example, probably the developer needs to add ../ into those *.eslintignore.

I got it about .gitignore does not rely on cwd.
Your point sounds reasonable to me.

Hmm, I will not oppose it, but I'm still afraid to do this change without a major version...

@IanVS
Copy link
Member

IanVS commented Aug 4, 2016

@mysticatea Ah yes I see, thanks. They would need to make a small change to their ignore files, but they will still be able to use that strategy. I was trying to think of a case where if we make this change then they are unable to work the same way at all, no matter how they change the ignore file. I can't think of anything like that so far.

@IanVS IanVS added the tsc agenda This issue will be discussed by ESLint's TSC at the next meeting label Aug 4, 2016
@platinumazure
Copy link
Member

I'm okay with this being treated as a breaking change too. I just want to see it done at some point 😄

Maybe we could also consider adding an option that would enable the old behavior? (Or put the proposed behavior behind an option, but... 😢)

@IanVS
Copy link
Member

IanVS commented Aug 4, 2016

Oh man, please no options! 😺

Seriously though, if this is a breaking change, I'd rather just wait for a major version.

@platinumazure
Copy link
Member

@IanVS Apologies, I meant as a separate enhancement later on. No need to add an option now. I'm also okay with treating this as a breaking change.

@ratherblue
Copy link
Author

ratherblue commented Aug 5, 2016

👍

@ratherblue
Copy link
Author

I know I opened the issue, but fwiw I also feel like it's a bug and I'm okay with it being a breaking change. A lot of SublimeText users are eager for that change, and I know the entire dev team at my previous job will be happy, too.

@ilyavolodin ilyavolodin added accepted There is consensus among the team that this change meets the criteria for inclusion breaking This change is backwards-incompatible and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion tsc agenda This issue will be discussed by ESLint's TSC at the next meeting labels Aug 18, 2016
@ilyavolodin
Copy link
Member

Accepted as a breaking change. That means we can't merge it before 4.0.

@kaelzhang
Copy link

I can help with the ignore-paths.js.

And any progress about #6783 ?

@nzakas nzakas modified the milestone: v4.0.0 Aug 19, 2016
@platinumazure
Copy link
Member

@eslint/eslint-team This has been sitting for a long time. Has anyone started work on this? Do we still want this in for 4.x?

If nobody has started working on it, I'm interested in working on this, but I want to close out some of my open PRs first (so if anyone else wants to start this, definitely go ahead).

@IanVS
Copy link
Member

IanVS commented Nov 23, 2016

Ah, I lost track of this. I'd like to take a shot at it, @platinumazure, but I am going to be traveling until next week. So if you've got time this weekend then go for it.

@IanVS
Copy link
Member

IanVS commented Nov 30, 2016

Working on this.

@ilyavolodin ilyavolodin added this to InProgress in v4.0.0 Feb 23, 2017
@not-an-aardvark not-an-aardvark moved this from InProgress to Ready in v4.0.0 Feb 23, 2017
@not-an-aardvark not-an-aardvark moved this from Ready to Merged in v4.0.0 Apr 5, 2017
@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 breaking This change is backwards-incompatible bug ESLint is working incorrectly core Relates to ESLint's core APIs and features
Projects
No open projects
v4.0.0
Merged
Development

No branches or pull requests

9 participants