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.

Sign up for GitHub

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

Jump to bottom

Fix: Always ignore defaults unless explicitly passed (fixes #5547) #5820

Merged
merged 3 commits into from
May 2, 2016
Merged

Fix: Always ignore defaults unless explicitly passed (fixes #5547) #5820

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0e13b3c
Fix: Always ignore defaults unless explicitly passed (fixes #5547)
IanVS Apr 11, 2016
f8308f2
Base ignore message on type of ignore
IanVS Apr 26, 2016
d829c92
Tweak ignore warning messages
IanVS Apr 28, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 19 additions & 6 deletions lib/cli-engine.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,18 +300,31 @@ function processFile(filename, configHelper, options) {

/**
* Returns result with warning by ignore settings
* @param {string} filePath File path of checked code
* @returns {Result} Result with single warning
* @param {string} filePath - File path of checked code
* @param {string} baseDir - Absolute path of base directory
* @returns {Result} Result with single warning
* @private
*/
function createIgnoreResult(filePath) {
function createIgnoreResult(filePath, baseDir) {
var message;

if (/^\./.test(path.basename(filePath))) {
message = "Hidden file ignored by default. Use '--ignore-pattern !.*' to override.";
Copy link
Member

@alberto alberto Apr 26, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EDITED: Correcting myself inline to avoid adding more confusion, sorry.

I think it would be better not to recommend to unignore everything, but just the current file in this case.

Also the ignore patterns have to be quoted in all cases. e.g. Use '--ignore-pattern \"!node_modules/*\"' to override.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was trying to keep this as simple as possible, knowing that the fancier I make it the more likely there will be bugs. It's already pretty ugly, and hopefully can be cleaned up when we refactor to simplify default ignores. For now, would you be opposed to a simpler message like:

File ignored by default.  Use a negated --ignore-pattern (like '!<filename>') to override."

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what I was proposing initially. Unfortunately it won't work for files in nested folders in node_modules or bower_components. You can't just unignore node_modules/foo/bar.js, you have to unignore node_modules/foo/ :/

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you misunderstand. I mean that message literally. Not using a variable. Let the user figure out what the pattern should be, after giving them the hint to use a !.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be clear, I'm proposing to adapt it like this:

var relativePath = path.relative(baseDir, filePath));

if (/^\./.test(path.basename(filePath))) {
  message = "File ignored by default.  Use '--ignore-pattern \"!" + relativePath + "\"' to override.";
} else if (baseDir && /^node_modules/.test(relativePath) {
  message = "File ignored by default. Use '--ignore-pattern \"!node_modules/*\"' to override.";
} else if (baseDir && /^bower_components/.test(path.relative(baseDir, filePath))) {
  message = "File ignored by default. Use '--ignore-pattern \"!bower_components/*\"' to override.";
} else {
  message = "File ignored because of a matching ignore pattern. Use '--no-ignore' to override.";
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did, but to be able to figure out the right pattern yourself (as I said, you can't simply unignore the file and I think that's what people would try given that message), you'd have to know we are using node_modules/* to ignore.

Copy link
Member Author

@IanVS IanVS Apr 26, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but I would like to avoid using relativePath directly in the message because baseDir may not always be provided and there may be other edge cases and conditions that cause trouble. I'd rather replace all of that logic with:

var isHidden = /^\./.test(path.basename(filePath));
var isInNodeModules = baseDir && /^node_modules/.test(path.relative(baseDir, filePath));
var isInBowerComponents = baseDir && /^bower_components/.test(path.relative(baseDir, filePath));

if (isHidden || isInNodeModules || isInBowerComponents) {
  message = "File ignored by default.  Use a negated --ignore-pattern (like '!<relative/path/to/filename>') to override.";
} else {
  message = "File ignored because of a matching ignore pattern. Use '--no-ignore' to override.";
}

This is still more complicated than I'd like and still relies on baseDir for node_modules and bower_components (no way around that), but does not require it for hidden files. The more explicit we are in the error message (by giving an exact flag to use), the greater chances for confusion if/when that suggestion does not work.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure !node_modules/foo/bar.js won't work? That would be annoying. . .

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can try it on your branch with:

eslint node_modules/escope/gulpfile.js --ignore-pattern '!node_modules/escope/gulpfile.js'

vs:

eslint node_modules/escope/gulpfile.js --ignore-pattern '!node_modules/*'

Also, bash needs single, not double quotes with "!" for some reason. :(

} else if (baseDir && /^node_modules/.test(path.relative(baseDir, filePath))) {
message = "File ignored by default. Use '--ignore-pattern !node_modules/*' to override.";
} else if (baseDir && /^bower_components/.test(path.relative(baseDir, filePath))) {
message = "File ignored by default. Use '--ignore-pattern !bower_components/*' to override.";
} else {
message = "File ignored because of a matching ignore pattern. Use --no-ignore to override.";
}

return {
filePath: path.resolve(filePath),
messages: [
{
fatal: false,
severity: 1,
message: "File ignored because of a matching ignore pattern. Use --no-ignore to override."
message: message
}
],
errorCount: 0,
Expand Down Expand Up @@ -612,7 +625,7 @@ CLIEngine.prototype = {
var hashOfConfig;

if (warnIgnored) {
results.push(createIgnoreResult(filename));
results.push(createIgnoreResult(filename, options.cwd));
return;
}

Expand Down Expand Up @@ -734,7 +747,7 @@ CLIEngine.prototype = {
}
if (filename && ignoredPaths.contains(filename)) {

results.push(createIgnoreResult(filename));
results.push(createIgnoreResult(filename, options.cwd));
} else {
results.push(processText(text, configHelper, filename, options.fix, options.allowInlineConfig));
}
Expand Down
12 changes: 8 additions & 4 deletions tests/lib/cli-engine.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,10 +261,11 @@ describe("CLIEngine", function() {
});

var report = engine.executeOnText("var bar = foo;", "node_modules/passing.js");
var expectedMsg = "File ignored by default. Use \'--ignore-pattern !node_modules/*\' to override.";

assert.equal(report.results.length, 1);
assert.equal(report.results[0].filePath, getFixturePath("node_modules/passing.js"));
assert.equal(report.results[0].messages[0].message, "File ignored because of a matching ignore pattern. Use --no-ignore to override.");
assert.equal(report.results[0].messages[0].message, expectedMsg);
});

});
Expand Down Expand Up @@ -391,11 +392,12 @@ describe("CLIEngine", function() {
});

var report = engine.executeOnFiles(["node_modules/foo.js"]);
var expectedMsg = "File ignored by default. Use \'--ignore-pattern !node_modules/*\' to override.";

assert.equal(report.results.length, 1);
assert.equal(report.results[0].errorCount, 0);
assert.equal(report.results[0].warningCount, 1);
assert.equal(report.results[0].messages[0].message, "File ignored because of a matching ignore pattern. Use --no-ignore to override.");
assert.equal(report.results[0].messages[0].message, expectedMsg);
});

it("should not check default ignored files without --no-ignore flag", function() {
Expand Down Expand Up @@ -433,11 +435,12 @@ describe("CLIEngine", function() {
});

var report = engine.executeOnFiles(["fixtures/files/.bar.js"]);
var expectedMsg = "Hidden file ignored by default. Use \'--ignore-pattern !.*\' to override.";

assert.equal(report.results.length, 1);
assert.equal(report.results[0].errorCount, 0);
assert.equal(report.results[0].warningCount, 1);
assert.equal(report.results[0].messages[0].message, "File ignored because of a matching ignore pattern. Use --no-ignore to override.");
assert.equal(report.results[0].messages[0].message, expectedMsg);
});

it("should check .hidden files if they are passed explicitly with --no-ignore flag", function() {
Expand Down Expand Up @@ -672,7 +675,8 @@ describe("CLIEngine", function() {

it("should return a warning when an explicitly given file is ignored", function() {
engine = new CLIEngine({
ignorePath: getFixturePath(".eslintignore")
ignorePath: getFixturePath(".eslintignore"),
cwd: getFixturePath()
});

var filePath = getFixturePath("passing.js");
Expand Down