Fix: Ignore hidden folders when resolving globs (fixes #8259) #8270
Conversation
|
LGTM |
|
@IanVS, thanks for your PR! By analyzing the history of the files in this pull request, we identified @not-an-aardvark, @gyandeeps and @mysticatea to be potential reviewers. |
This is a performance improvement, but should not be a change to linting functionality. This is similar to avoiding glob resolution within `node_modules`. With this change, `node-glob` will not attempt to find all files within `.dotfolder` folders. Similar to `node_modules`, this can be overridden in a user’s ignore file or in an ignore-pattern.
|
LGTM |
| if (this.options.dotfiles !== true) { | ||
|
|
||
| // Ignore hidden folders. (This cannot be ".*", or else it's not possible to unignore hidden files) | ||
| ig.add([".*/*", "!../"]); |
There was a problem hiding this comment.
I think it could be .*/. It's just that you would have to unignore the folder you want to lint e.g !.foo/. Unless the issue is with glob syntax, which I am not so familiar with.
There was a problem hiding this comment.
That's right, it's not possible to unignore an ignored folder, which is why I needed it this way.
There was a problem hiding this comment.
I think it is not good to ig.add('!../'), because .gitignore only cares about the current directory and subtle directories.
Even if it works to do this, it is the side effect of node-ignore however.
There was a problem hiding this comment.
There might be a better workaround to make sure ../ is excluded in the glob result (May be it already is which I haven't confirmed yet).
There was a problem hiding this comment.
Yeah, I added that pattern a while ago because the patterns in .eslintignore currently are applied against the CWD, not the file's directory (until #7678 is merged). I think we could consider removing that pattern in that PR, since you're right, from then on, the patterns will be relative to the ignore file's location, and the ignore file must be at the project root (unless we make changes for #8090).
It might be interesting to do a search for .eslintignore files on github which contain ../. Seems like that should be possible, but I'm not exactly sure how.
There was a problem hiding this comment.
By the way, thanks for reviewing, @kaelzhang!
not-an-aardvark
added
accepted
|
It might be good for @kaelzhang to double-check this as well. |
| if (this.options.dotfiles !== true) { | ||
|
|
||
| // Ignore hidden folders. (This cannot be ".*", or else it's not possible to unignore hidden files) | ||
| ig.add([".*/*", "!../"]); |
There was a problem hiding this comment.
As a separate issue (probably for a different PR) maybe we should name this variable to something other than ig. I was having trouble understanding what this code was doing because it's hard to tell what ig.add means conceptually.
eslint-deprecated
bot
added
the
archived due to age
What is the purpose of this pull request? (put an "X" next to item)
[ ] Documentation update
[X] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:
What changes did you make? (Give an overview)
This is a performance improvement, but should not be a change to linting functionality. This is similar to avoiding glob resolution within
node_modules. With this change,node-globwill not attempt to find all files within.dotfolderfolders. Similar tonode_modules, this can be overridden in a user’s ignore file or in an ignore-pattern, and is also prevented if thedotfilesoption istrue.Is there anything you'd like reviewers to focus on?
It would be good to double-check our tests around hidden files and folders, as well as perhaps manually trying a few situations. From everything I tested, this works exactly like before, except faster. 🐎