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

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

Merged
merged 3 commits into from
May 2, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
38 changes: 25 additions & 13 deletions lib/cli-engine.js
Original file line number Diff line number Diff line change
Expand Up @@ -300,18 +300,34 @@ 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;
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) {
message = "File ignored by default. Use a negated ignore pattern (like \"--ignore-pattern \'!<relative/path/to/filename>\'\") to override.";
} else if (isInNodeModules) {
message = "File ignored by default. Use \"--ignore-pattern \'!node_modules/*\'\" to override.";
} else if (isInBowerComponents) {
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 +628,7 @@ CLIEngine.prototype = {
var hashOfConfig;

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

Expand Down Expand Up @@ -732,9 +748,9 @@ CLIEngine.prototype = {
if (filename && !isAbsolute(filename)) {
filename = path.resolve(options.cwd, filename);
}
if (filename && (options.ignore !== false) && ignoredPaths.contains(filename)) {
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 Expand Up @@ -770,12 +786,8 @@ CLIEngine.prototype = {
var ignoredPaths;
var resolvedPath = path.resolve(this.options.cwd, filePath);

if (this.options.ignore) {
ignoredPaths = new IgnoredPaths(this.options);
return ignoredPaths.contains(resolvedPath);
}

return false;
ignoredPaths = new IgnoredPaths(this.options);
return ignoredPaths.contains(resolvedPath);
},

getFormatter: CLIEngine.getFormatter
Expand Down
16 changes: 10 additions & 6 deletions lib/util/glob-util.js
Original file line number Diff line number Diff line change
Expand Up @@ -124,21 +124,25 @@ function listFilesToProcess(globPatterns, options) {
var ignored = false;
var isSilentlyIgnored;

if (ignoredPaths.contains(filename, "default")) {
ignored = (options.ignore !== false) && shouldWarnIgnored;
isSilentlyIgnored = !shouldWarnIgnored;
}

if (options.ignore !== false) {
if (ignoredPaths.contains(filename, "default")) {
isSilentlyIgnored = true;
}
if (ignoredPaths.contains(filename, "custom")) {
if (shouldWarnIgnored) {
ignored = true;
} else {
isSilentlyIgnored = true;
}
}
if (isSilentlyIgnored && !ignored) {
return;
}
}

if (isSilentlyIgnored && !ignored) {
return;
}

if (added[filename]) {
return;
}
Expand Down
1 change: 0 additions & 1 deletion tests/fixtures/cli-engine/.eslintignore

This file was deleted.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions tests/fixtures/cli-engine/nested_node_modules/passing.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions tests/fixtures/cli-engine/node_modules/foo.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

98 changes: 83 additions & 15 deletions tests/lib/cli-engine.js
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ describe("CLIEngine", function() {
assert.equal(report.warningCount, 1);
assert.equal(report.results[0].filePath, getFixturePath("passing.js"));
assert.equal(report.results[0].messages[0].severity, 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, "File ignored because of a matching ignore pattern. Use \"--no-ignore\" to override.");
assert.isUndefined(report.results[0].messages[0].output);
assert.equal(report.results[0].errorCount, 0);
assert.equal(report.results[0].warningCount, 1);
Expand Down Expand Up @@ -252,6 +252,22 @@ describe("CLIEngine", function() {
});
});

// https://github.com/eslint/eslint/issues/5547
it("should respect default ignore rules, even with --no-ignore", function() {
Copy link
Member

Choose a reason for hiding this comment

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

Same question as above here regarding explicitly passed files.


engine = new CLIEngine({
cwd: getFixturePath(),
ignore: false
});

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, expectedMsg);
});

});

describe("executeOnFiles()", function() {
Expand Down Expand Up @@ -369,39 +385,81 @@ describe("CLIEngine", function() {
assert.equal(report.results[1].messages.length, 0);
});

it("should report on all files passed explicitly, even if ignored by default", function() {

engine = new CLIEngine({
cwd: getFixturePath("cli-engine")
});

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, expectedMsg);
});

it("should not check default ignored files without --no-ignore flag", function() {

engine = new CLIEngine({
cwd: path.join(fixtureDir, "..")
cwd: getFixturePath("cli-engine")
});

var report = engine.executeOnFiles(["node_modules"]);

assert.equal(report.results.length, 0);
});

// https://github.com/eslint/eslint/issues/5547
it("should not check node_modules files even with --no-ignore flag", function() {

engine = new CLIEngine({
cwd: getFixturePath("cli-engine"),
ignore: false
});

var report = engine.executeOnFiles(["fixtures/files/node_modules/.bar.js"]);
var report = engine.executeOnFiles(["node_modules"]);

assert.equal(report.results.length, 0);
});

it("should not check .hidden files if they are passed explicitly without --no-ignore flag", function() {

engine = new CLIEngine({
cwd: path.join(fixtureDir, "..")
cwd: getFixturePath(".."),
useEslintrc: false,
rules: {
quotes: [2, "single"]
}
});

var report = engine.executeOnFiles(["fixtures/files/.bar.js"]);
var expectedMsg = "File ignored by default. Use a negated ignore pattern (like \"--ignore-pattern \'!<relative/path/to/filename>\'\") to override.";

assert.equal(report.results.length, 0);
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, expectedMsg);
});

it("should check .hidden files if they are passed explicitly with --no-ignore flag", function() {

engine = new CLIEngine({
cwd: getFixturePath(".."),
ignore: false,
cwd: path.join(fixtureDir, "..")
useEslintrc: false,
rules: {
quotes: [2, "single"]
}
});

var report = engine.executeOnFiles(["fixtures/files/.bar.js"]);

assert.equal(report.results.length, 1);
assert.equal(report.results[0].messages.length, 0);
assert.equal(report.results[0].warningCount, 0);
assert.equal(report.results[0].errorCount, 1);
assert.equal(report.results[0].messages[0].ruleId, "quotes");
});

it("should report zero messages when given a pattern with a .js and a .js2 file", function() {
Expand Down Expand Up @@ -573,17 +631,17 @@ describe("CLIEngine", function() {
});

// https://github.com/eslint/eslint/issues/3788
it("should ignore one-level down node_modules when ignore file has **/_node_modules in it", function() {
it("should ignore one-level down node_modules when ignore file has 'node_modules/' in it", function() {
engine = new CLIEngine({
ignorePath: getFixturePath("cli-engine/.eslintignore"),
ignorePath: getFixturePath("cli-engine", "nested_node_modules", ".eslintignore"),
useEslintrc: false,
rules: {
quotes: [2, "double"]
},
cwd: path.join(fixtureDir, "..")
cwd: getFixturePath("cli-engine", "nested_node_modules")
});

var report = engine.executeOnFiles(["./fixtures/cli-engine/"]);
var report = engine.executeOnFiles(["."]);

assert.equal(report.results.length, 1);
assert.equal(report.results[0].errorCount, 0);
Expand Down Expand Up @@ -617,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 All @@ -629,7 +688,7 @@ describe("CLIEngine", function() {
assert.equal(report.warningCount, 1);
assert.equal(report.results[0].filePath, filePath);
assert.equal(report.results[0].messages[0].severity, 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, "File ignored because of a matching ignore pattern. Use \"--no-ignore\" to override.");
assert.equal(report.results[0].errorCount, 0);
assert.equal(report.results[0].warningCount, 1);
});
Expand Down Expand Up @@ -1854,15 +1913,24 @@ describe("CLIEngine", function() {
assert.isFalse(engine.isPathIgnored("passing.js"));
});

it("should always return false if ignoring is disabled", function() {
it("should return false if ignoring is disabled", function() {
var engine = new CLIEngine({
ignore: false,
ignorePath: getFixturePath(".eslintignore2"),
cwd: getFixturePath()
});

assert.isFalse(engine.isPathIgnored("undef.js"));
assert.isFalse(engine.isPathIgnored("passing.js"));
});

// https://github.com/eslint/eslint/issues/5547
it("should return true for default ignores even if ignoring is disabled", function() {
var engine = new CLIEngine({
ignore: false,
cwd: getFixturePath("cli-engine")
});

assert.isTrue(engine.isPathIgnored("node_modules/foo.js"));
});

});
Expand Down
44 changes: 44 additions & 0 deletions tests/lib/util/glob-util.js
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,50 @@ describe("globUtil", function() {
]);
});

it("should silently ignore default ignored files if not passed explicitly", function() {
var directory = getFixturePath("glob-util", "hidden");
var patterns = [directory];
var result = globUtil.listFilesToProcess(patterns, {
cwd: getFixturePath()
});

assert.equal(result.length, 0);
});

it("should ignore and warn for default ignored files when passed explicitly", function() {
Copy link
Member

Choose a reason for hiding this comment

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

@nzakas Is this the expected behavior? I understood otherwise from these comments

Copy link
Member

Choose a reason for hiding this comment

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

I think that's the big we are fixing here, no?

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 we may be getting mixed up on a few related issues. This PR is to fix #5547, which is to say, to keep ignoring default-ignored files even if the --no-ignore flag is passed.

The current behavior has always been to print a warning if an explicitly passed file is ignored due to an ignore rule. #4828 (comment) is a separate issue and will need greater structural changes to accomplish.

Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure we should add a test for a behaviour we think is a bug, but yeah, it's a different issue.

var filename = getFixturePath("glob-util", "hidden", ".foo.js");
var patterns = [filename];
var result = globUtil.listFilesToProcess(patterns, {
cwd: getFixturePath()
});

assert.equal(result.length, 1);
assert.deepEqual(result[0], { filename: filename, ignored: true });
});

it("should silently ignore default ignored files if not passed explicitly even if ignore is false", function() {
var directory = getFixturePath("glob-util", "hidden");
var patterns = [directory];
var result = globUtil.listFilesToProcess(patterns, {
cwd: getFixturePath(),
ignore: false
});

assert.equal(result.length, 0);
});

it("should not ignore default ignored files when passed explicitly if ignore is false", function() {
var filename = getFixturePath("glob-util", "hidden", ".foo.js");
var patterns = [filename];
var result = globUtil.listFilesToProcess(patterns, {
cwd: getFixturePath(),
ignore: false
});

assert.equal(result.length, 1);
assert.deepEqual(result[0], { filename: filename, ignored: false });
});

it("should not return a file which does not exist", function() {
var patterns = ["tests/fixtures/glob-util/hidden/bar.js"];
var result = globUtil.listFilesToProcess(patterns);
Expand Down