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

cli.js and ignore list (question) #779

Closed
ainthek opened this issue Dec 18, 2012 · 3 comments
Closed

cli.js and ignore list (question) #779

ainthek opened this issue Dec 18, 2012 · 3 comments

Comments

@ainthek
Copy link

ainthek commented Dec 18, 2012

ignoreFile = _searchFile(".jshintignore");
console.log("ignoreFile:",ignoreFile);
if (ignoreFile) {
ignores = fs.readFileSync(ignoreFile, "utf8").split("\n")
.filter(function (line) {
return !!line.trim();
});
//.map(function (line) {
// return path.resolve(path.dirname(ignoreFile), line.trim());
//});
}

please can you explain the intention of map section and prefixing all minimatch expressions with dir name ? I think it makes them not to work as expected when running from other than project file.

Please extend .jshintignore docs.

@ArturDorochowicz
Copy link

@ainthek
I think you are right that there is a problem here, though I still need to come up with an example (i.e. proof). This should be visible with negative match pattern, I reckon.

The original change was introduced in the node-jshint repository at jshint/node-jshint@c170fec#L0R164. Not sure how this change is related to the intent of the commit as indicated by the commit message.
Then for some time, I think, it did not work at all (due to matching relative file path against full pattern path), until changes in this repository introduced in #777 which introduce resolving of the tested path.

@valueof
Copy link
Member

valueof commented Jan 18, 2013

This makes sure that .jshintignore entries are relative to the file itself. If you want to ignore all files with name, let's say, wat.js you just need to put **/wat.js. I think everything works as expected here.

@valueof valueof closed this as completed Jan 18, 2013
@ArturDorochowicz
Copy link

@antonkovalyov
Sorry, I should have followed with the example earlier.
I think it seems to work in the common cases - as you say, but breaks at least one kind of pattern due to modifying of the original pattern. The pattern that for sure doesn't work is negated pattern, for instance, given a structure as this:

.jshintignore
file1.js
file2.js

and .jshintignore being:

!file1.js

when jshint is run in that directory and on that directory, it should scan only file1.js, but actually scans both files.
The issue is that minimatch only recognizes negative patterns when the exclamation mark is the first character, but jshint actually provides minimatch with something like (I'm on Windows):

minimatch('d:\\dev\\_\\file1.js', 'd:\\dev\\_\\!file1.js', {nocase:true})
and
minimatch('d:\\dev\\_\\file2.js', 'd:\\dev\\_\\!file1.js', {nocase:true})

and they both evaluate to false, because the pattern is not recognized as negated pattern by minimatch.

BTW, I started checking tests for this (tests\cli.js - testIgnoreFile) and it seems to me that the second part of the test (from https://github.com/jshint/jshint/blob/master/tests/cli.js#L248 onward) doesn't do anything - it sets up the stubs, but doesn't exercise cli with them.

@valueof valueof reopened this Jan 21, 2013
@valueof valueof closed this as completed in dd060c7 Jul 2, 2013
danielctull pushed a commit to danielctull-forks/jshint that referenced this issue Jan 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants