Skip to content

Commit

Permalink
[[FIX]] Correct CLI's indentation offset logic
Browse files Browse the repository at this point in the history
Don't attempt to indent errors using data from other files, during
extract with multiple files

This change will fix the report of `col NaN` and the problem of `NaN` in
`error.character` while checking JS code extracted from non-JS files,
when more than one file has been passed in and their lengths don't
match.

The function `extractOffsets` finds the number of characters a JS block
must be shifted to show the correct character column for an error. The
result is an array consisting of numbers, for the block it checked and,
`undefined` for the lines it didn't have to look at.  `cli.run` executes
`extractOffsets` on each input file but wrongly tries to indent all the
errors found so far, across files, using the current file's offsets.
This commit makes sure `cli.run` only indents errors of a file against
the offsets fetched for the same file.

This commit also makes sure that we don't change or ruin the value of
`error.character` unnecessarily.

I have chosen to trust that `extractOffsets` will return an array with
only numbers (as a result of Array.length) and falsy values, to avoid
the unnecessary type conversion.

A test named `usingMultipleFiles` has been introduced to cli.

Fixes #2778
  • Loading branch information
AMoo-Miki authored and jugglinmike committed Apr 19, 2016
1 parent 2f127ef commit 47daf76
Show file tree
Hide file tree
Showing 2 changed files with 56 additions and 9 deletions.
14 changes: 8 additions & 6 deletions src/cli.js
Original file line number Diff line number Diff line change
Expand Up @@ -642,6 +642,7 @@ var exports = {
files.forEach(function(file) {
var config = opts.config || exports.getConfig(file);
var code;
var errors = [];

try {
code = shjs.cat(file);
Expand All @@ -652,19 +653,20 @@ var exports = {

mergeCLIPrereq(config);

lint(extract(code, opts.extract), results, config, data, file);
lint(extract(code, opts.extract), errors, config, data, file);

if (results.length) {
if (errors.length) {
var offsets = extractOffsets(code, opts.extract);
if (offsets && offsets.length) {
results.forEach(function(errorInfo) {
errors.forEach(function(errorInfo) {
var line = errorInfo.error.line;
if (line >= 0 && line < offsets.length) {
var offset = +offsets[line];
errorInfo.error.character += offset;
if (line >= 0 && line < offsets.length && offsets[line]) {
errorInfo.error.character += offsets[line];
}
});
}

results = results.concat(errors);
}
});

Expand Down
51 changes: 48 additions & 3 deletions tests/cli.js
Original file line number Diff line number Diff line change
Expand Up @@ -633,7 +633,7 @@ exports.group = {

test.done();
},

testMultipleIgnores: function (test) {
var run = this.sinon.stub(cli, "run");
var dir = __dirname + "/../examples/";
Expand All @@ -642,10 +642,10 @@ exports.group = {
cli.interpret([
"node", "jshint", "file.js", "--exclude=foo.js,bar.js"
]);

test.equal(run.args[0][0].ignores[0], path.resolve(dir, "foo.js"));
test.equal(run.args[0][0].ignores[1], path.resolve(dir, "bar.js"));

test.done();
},

Expand Down Expand Up @@ -1152,6 +1152,51 @@ exports.extract = {
test.done();
},

usingMultipleFiles: function (test) {
var rep = require("../examples/reporter.js");
var errors = [];
this.sinon.stub(rep, "reporter", function (res) {
errors = errors.concat(res);
});

var dir = __dirname + "/../examples/";
this.sinon.stub(process, "cwd").returns(dir);

var html = [
"<script type='text/javascript'>",
" a()",
"</script>",
].join("\n");

this.sinon.stub(shjs, "cat")
.withArgs(sinon.match(/indent\.html$/)).returns(html)
.withArgs(sinon.match(/another\.html$/)).returns("\n\n<script>a && a();</script>");

this.sinon.stub(shjs, "test")
.withArgs("-e", sinon.match(/indent\.html$/)).returns(true)
.withArgs("-e", sinon.match(/another\.html$/)).returns(true);

cli.interpret([
"node", "jshint", "indent.html", "another.html", "--extract", "auto", "--reporter=reporter.js"
]);
test.equal(cli.exit.args[0][0], 2);

test.equal(errors.length, 2, "found two errors");
var lintError = errors[0].error;
test.ok(lintError, "have error object");
test.equal(lintError.code, "W033", "found missing semicolon warning");
test.equal(lintError.line, 2, "misaligned line");
test.equal(lintError.character, 6, "first misaligned character at column 2");

lintError = errors[1].error;
test.ok(lintError, "have error object");
test.equal(lintError.code, "W030", "found an expression warning");
test.equal(lintError.line, 3, "misaligned line");
test.equal(lintError.character, 8, "first misaligned character at column 8");

test.done();
},

"\\r\\n as line terminator (gh-2825)": function (test) {
var html = [
"<script>",
Expand Down

0 comments on commit 47daf76

Please sign in to comment.