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: Incorrectly warning about ignored files #3666

Merged
merged 1 commit into from Sep 8, 2015
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
63 changes: 44 additions & 19 deletions lib/cli-engine.js
Expand Up @@ -274,17 +274,37 @@ function isErrorMessage(message) {
*
* Also makes sure all path separators are POSIX style for `glob` compatibility.
*
* @param {string} pathname The directory path to be modified
* @returns {string} The glob path or the file path itself
* @param {string[]} [extensions] An array of accepted extensions
* @returns {Function} A function that takes a pathname and returns a glob that
* matches all files with the provided extensions if
* pathname is a directory.
*/
function processPath(pathname) {
var newPath = pathname;
function processPath(extensions) {
var suffix = "/**";

if (shell.test("-d", pathname)) {
newPath = path.join(pathname, "**");
if (extensions) {
if (extensions.length === 1) {
suffix += "/*." + extensions[0];
} else {
suffix += "/*.{" + extensions.join(",") + "}";
}
}

return newPath.replace(/\\/g, "/");
/**
* A function that converts a directory name to a glob pattern
*
* @param {string} pathname The directory path to be modified
* @returns {string} The glob path or the file path itself
*/
return function(pathname) {
var newPath = pathname;

if (shell.test("-d", pathname)) {
newPath = pathname + suffix;
}

return newPath.replace(/\\/g, "/");
};
}


Expand Down Expand Up @@ -409,32 +429,33 @@ CLIEngine.prototype = {

/**
* Executes the current configuration on an array of file and directory names.
* @param {string[]} files An array of file and directory names.
* @param {string[]} patterns An array of file and directory names.
* @returns {Object} The results for all files that were linted.
*/
executeOnFiles: function(files) {
executeOnFiles: function(patterns) {
var results = [],
processed = {},
options = this.options,
fileCache = this._fileCache, // eslint-disable-line no-underscore-dangle
configHelper = new Config(options),
ignoredPaths = options.ignore ? IgnoredPaths.load(options).patterns : [],
ignoredPaths = IgnoredPaths.load(options),
ignoredPathsList = ignoredPaths.patterns || [],
extensions = options.extensions.map(function(ext) {
return ext.charAt(0) === "." ? ext : "." + ext;
return ext.charAt(0) === "." ? ext.substr(1) : ext;
}),
globOptions,
stats,
startTime;

startTime = Date.now();

files = files.map(processPath);
ignoredPaths = ignoredPaths.map(processPath);
patterns = patterns.map(processPath(extensions));
ignoredPathsList = ignoredPathsList.map(processPath());

globOptions = {
nodir: true,
realpath: true,
ignore: ignoredPaths
ignore: ignoredPathsList
};

/**
Expand All @@ -444,7 +465,7 @@ CLIEngine.prototype = {
* @returns {void}
*/
function executeOnFile(filename) {
if (extensions.indexOf(path.extname(filename)) === -1 || processed[filename]) {
if (processed[filename]) {
return;
}

Expand Down Expand Up @@ -496,15 +517,19 @@ CLIEngine.prototype = {
results.push(res);
}

files.forEach(function(pattern) {
glob.sync(pattern, globOptions).forEach(executeOnFile);
patterns.forEach(function(pattern) {
if (shell.test("-f", pattern) && !ignoredPaths.contains(pattern)) {
Copy link
Member

Choose a reason for hiding this comment

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

Why not just use fs insead of shell. And that way we can get rid of the shell dependency and move it back to devDependencies.

What is the use case where ignoredPaths.contains(pattern) will be true under the condition that its a file?

Copy link
Member

Choose a reason for hiding this comment

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

We use ShellJS in a few places already.

Copy link
Member

Choose a reason for hiding this comment

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

shelljs is only used in here at 2 spots where we can easily use fs. By doing that we can move shelljs back to devDependencies section.

Copy link
Member

Choose a reason for hiding this comment

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

This was already discussed in a previous issue. fs.exists is deprecated and is being removed., so we can't rely on it. What is your alternative?

Copy link
Member

Choose a reason for hiding this comment

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

we can use fs.statSync(pattern).isFile() to check for if its a file.
https://nodejs.org/api/fs.html#fs_class_fs_stats

Copy link
Member Author

Choose a reason for hiding this comment

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

That requires a try-catch block. This is the reason why @nzakas suggested using shelljs.

Copy link
Member

Choose a reason for hiding this comment

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

got it. Thanks
@BYK What is the use case where ignoredPaths.contains(pattern) will be true under the condition that its a file?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure what you are asking. Can you clarify?

This new if block I added checks if the pattern passed is actually a file and not ignored by the .eslintignore file. If that is the case it skips glob and directly executes the file.

Copy link
Member

Choose a reason for hiding this comment

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

What do i need to have in my eslintignore file to make these 2 conditions true

  • shell.test("-f", pattern) === true
  • ignoredPaths.contains(pattern) === true

Copy link
Member Author

Choose a reason for hiding this comment

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

executeOnFile(fs.realpathSync(pattern));
} else {
glob.sync(pattern, globOptions).forEach(executeOnFile);
}
});

// only warn for files explicitly passed on the command line
if (options.ignore) {
files.forEach(function(file) {
patterns.forEach(function(file) {
var fullPath = path.resolve(file);
if (shell.test("-e", fullPath) && !processed[fullPath]) {
if (shell.test("-f", fullPath) && !processed[fullPath]) {
results.push(createIgnoreResult(file));
}
});
Expand Down
9 changes: 9 additions & 0 deletions tests/lib/cli-engine.js
Expand Up @@ -304,6 +304,15 @@ describe("CLIEngine", function() {
assert.equal(report.results[1].warningCount, 0);
});

it("should process when file is given by not specifying extensions", function() {

engine = new CLIEngine();

var report = engine.executeOnFiles(["tests/fixtures/files/foo.js2"]);
assert.equal(report.results.length, 1);
assert.equal(report.results[0].messages.length, 0);
});

it("should return zero messages when given a config with environment set to browser", function() {

engine = new CLIEngine({
Expand Down