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: Warn for deprecation in Node output (fixes #7443) #10953

Merged
merged 8 commits into from Oct 30, 2018
Merged

Fix: Warn for deprecation in Node output (fixes #7443) #10953

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3c049d0
Fix: Warn for deprecation in Node output (fixes #7443)
calling Oct 7, 2018
8375110
Merge branch 'master' into issue/7443
calling Oct 19, 2018
5d0fecf
Reduce complexity of createRuleWarnings()
calling Oct 19, 2018
cd0ee6e
Specify the usedDeprecatedRules object properties in the docs
calling Oct 19, 2018
3c72430
Add safety checks for createRuleWarnings()
calling Oct 19, 2018
7e47529
Document replacedBy in working-with-rules
calling Oct 19, 2018
475a063
Merge branch 'master' into issue/7443
calling Oct 28, 2018
f7eb619
Move meta.docs.replacedBy to meta.replacedBy
calling Oct 28, 2018
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
7 changes: 5 additions & 2 deletions docs/developer-guide/nodejs-api.md
View file
Open in desktop
Expand Up @@ -416,7 +416,8 @@ The return value is an object containing the results of the linting operation. H
errorCount: 1,
warningCount: 0,
fixableErrorCount: 1,
fixableWarningCount: 0
fixableWarningCount: 0,
usedDeprecatedRules: []
}
```

Expand Down Expand Up @@ -474,6 +475,7 @@ var report = cli.executeOnFiles(["myfile.js", "lib/"]);
warningCount: 0,
fixableErrorCount: 1,
fixableWarningCount: 0,
usedDeprecatedRules: []
}
```

Expand Down Expand Up @@ -505,6 +507,7 @@ If the operation ends with a parsing error, you will get a single message for th
warningCount: 0,
fixableErrorCount: 0,
fixableWarningCount: 0,
usedDeprecatedRules: []
}
```

Expand All @@ -516,7 +519,7 @@ The top-level report object has a `results` array containing all linting results
* `source` - The source code for the given file. This property is omitted if this file has no errors/warnings or if the `output` property is present.
* `output` - The source code for the given file with as many fixes applied as possible, so you can use that to rewrite the files if necessary. This property is omitted if no fix is available.

The top-level report object also has `errorCount` and `warningCount` which give the exact number of errors and warnings respectively on all the files.
The top-level report object also has `errorCount` and `warningCount` which give the exact number of errors and warnings respectively on all the files. Additionally, `usedDeprecatedRules` signals any deprecated rules used and their replacement (if available).
Copy link
Member

Choose a reason for hiding this comment

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

Could you be more specific in this documentation about what the shape of an object in usedDeprecatedRules would look like (namely, that it has a ruleId and replacedBy property)?


Once you get a report object, it's up to you to determine how to output the results. Fixes will not be automatically applied to the files, even if you set `fix: true` when constructing the `CLIEngine` instance. To apply fixes to the files, call [`outputFixes`](#cliengineoutputfixes).

Expand Down
29 changes: 28 additions & 1 deletion lib/cli-engine.js
View file
Open in desktop
Expand Up @@ -272,6 +272,30 @@ function createIgnoreResult(filePath, baseDir) {
};
}

/**
* Produces rule warnings (i.e. deprecation) from configured rules
* @param {Object} rules - Rules configured
* @param {Map} loadedRules - Map of loaded rules
* @returns {Object} Contains rule warnings
* @private
*/
function createRuleWarnings(rules, loadedRules) {
const ruleWarnings = { usedDeprecatedRules: [] };

if (!rules) {
Copy link
Member

Choose a reason for hiding this comment

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

I know this isn't a terribly complex function, but I wonder if we could reduce the complexity a bit by restructuring it as:

const ruleWarnings = { usedDeprecatedRules: [] };

if (rules) {
    Object.keys(rules).forEach(name => {
        const loadedRule = loadedRules.get(name);

         if (loadedRule.meta && loadedRule.meta.deprecated) {
            ruleWarnings.usedDeprecatedRules.push({ ruleId: name, replacedBy: loadedRule.meta.docs.replacedBy });
        }
    });
}

return ruleWarnings;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem!

return ruleWarnings;
}

Object.keys(rules).forEach(name => {
const loadedRule = loadedRules.get(name);

if (loadedRule.meta && loadedRule.meta.deprecated) {
ruleWarnings.usedDeprecatedRules.push({ ruleId: name, replacedBy: loadedRule.meta.docs.replacedBy });
}
});

return ruleWarnings;
}

/**
* Checks if the given message is an error message.
Expand Down Expand Up @@ -555,14 +579,17 @@ class CLIEngine {

const stats = calculateStatsPerRun(results);

const ruleWarnings = createRuleWarnings(this.options.rules, this.getRules());

debug(`Linting complete in: ${Date.now() - startTime}ms`);

return {
results,
errorCount: stats.errorCount,
warningCount: stats.warningCount,
fixableErrorCount: stats.fixableErrorCount,
fixableWarningCount: stats.fixableWarningCount
fixableWarningCount: stats.fixableWarningCount,
usedDeprecatedRules: ruleWarnings.usedDeprecatedRules
};
}

Expand Down
155 changes: 90 additions & 65 deletions tests/lib/cli-engine.js
View file
Open in desktop
Expand Up @@ -1375,6 +1375,33 @@ describe("CLIEngine", () => {
assert.strictEqual(report.results[0].messages.length, 0);
});

it("should warn when deprecated rules are configured", () => {
engine = new CLIEngine({
cwd: originalDir,
configFile: ".eslintrc.js",
rules: { "indent-legacy": 1 }
});

const report = engine.executeOnFiles(["lib/cli*.js"]);

assert.deepStrictEqual(
report.usedDeprecatedRules,
[{ ruleId: "indent-legacy", replacedBy: ["indent"] }]
);
});

it("should not warn when deprecated rules are not configured", () => {
engine = new CLIEngine({
cwd: originalDir,
configFile: ".eslintrc.js",
rules: { indent: 1 }
});

const report = engine.executeOnFiles(["lib/cli*.js"]);

assert.deepStrictEqual(report.usedDeprecatedRules, []);
});

describe("Fix Mode", () => {

it("should return fixed text on multiple files when in fix mode", () => {
Expand Down Expand Up @@ -1407,70 +1434,68 @@ describe("CLIEngine", () => {
const report = engine.executeOnFiles([path.resolve(fixtureDir, `${fixtureDir}/fixmode`)]);

report.results.forEach(convertCRLF);
assert.deepStrictEqual(report, {
results: [
{
filePath: fs.realpathSync(path.resolve(fixtureDir, "fixmode/multipass.js")),
messages: [],
errorCount: 0,
warningCount: 0,
fixableErrorCount: 0,
fixableWarningCount: 0,
output: "true ? \"yes\" : \"no\";\n"
},
{
filePath: fs.realpathSync(path.resolve(fixtureDir, "fixmode/ok.js")),
messages: [],
errorCount: 0,
warningCount: 0,
fixableErrorCount: 0,
fixableWarningCount: 0
},
{
filePath: fs.realpathSync(path.resolve(fixtureDir, "fixmode/quotes-semi-eqeqeq.js")),
messages: [
{
column: 9,
line: 2,
message: "Expected '===' and instead saw '=='.",
messageId: "unexpected",
nodeType: "BinaryExpression",
ruleId: "eqeqeq",
severity: 2
}
],
errorCount: 1,
warningCount: 0,
fixableErrorCount: 0,
fixableWarningCount: 0,
output: "var msg = \"hi\";\nif (msg == \"hi\") {\n\n}\n"
},
{
filePath: fs.realpathSync(path.resolve(fixtureDir, "fixmode/quotes.js")),
messages: [
{
column: 18,
line: 1,
endColumn: 21,
endLine: 1,
message: "'foo' is not defined.",
nodeType: "Identifier",
ruleId: "no-undef",
severity: 2
}
],
errorCount: 1,
warningCount: 0,
fixableErrorCount: 0,
fixableWarningCount: 0,
output: "var msg = \"hi\" + foo;\n"
}
],
errorCount: 2,
warningCount: 0,
fixableErrorCount: 0,
fixableWarningCount: 0
});
assert.deepStrictEqual(report.results, [
{
filePath: fs.realpathSync(path.resolve(fixtureDir, "fixmode/multipass.js")),
messages: [],
errorCount: 0,
warningCount: 0,
fixableErrorCount: 0,
fixableWarningCount: 0,
output: "true ? \"yes\" : \"no\";\n"
},
{
filePath: fs.realpathSync(path.resolve(fixtureDir, "fixmode/ok.js")),
messages: [],
errorCount: 0,
warningCount: 0,
fixableErrorCount: 0,
fixableWarningCount: 0
},
{
filePath: fs.realpathSync(path.resolve(fixtureDir, "fixmode/quotes-semi-eqeqeq.js")),
messages: [
{
column: 9,
line: 2,
message: "Expected '===' and instead saw '=='.",
messageId: "unexpected",
nodeType: "BinaryExpression",
ruleId: "eqeqeq",
severity: 2
}
],
errorCount: 1,
warningCount: 0,
fixableErrorCount: 0,
fixableWarningCount: 0,
output: "var msg = \"hi\";\nif (msg == \"hi\") {\n\n}\n"
},
{
filePath: fs.realpathSync(path.resolve(fixtureDir, "fixmode/quotes.js")),
messages: [
{
column: 18,
line: 1,
endColumn: 21,
endLine: 1,
message: "'foo' is not defined.",
nodeType: "Identifier",
ruleId: "no-undef",
severity: 2
}
],
errorCount: 1,
warningCount: 0,
fixableErrorCount: 0,
fixableWarningCount: 0,
output: "var msg = \"hi\" + foo;\n"
}
]);
assert.strictEqual(report.errorCount, 2);
assert.strictEqual(report.warningCount, 0);
assert.strictEqual(report.fixableErrorCount, 0);
assert.strictEqual(report.fixableWarningCount, 0);
});

it("should run autofix even if files are cached without autofix results", () => {
Expand Down Expand Up @@ -2117,7 +2142,7 @@ describe("CLIEngine", () => {
assert.deepStrictEqual(result, cachedResult, "the result is the same regardless of using cache or not");

// assert the file was not processed because the cache was used
assert.isFalse(spy.called, "the file was not loaded because it used the cache");
assert.isFalse(spy.calledWith(file), "the file was not loaded because it used the cache");
});

it("should remember the files from a previous run and do not operate on then if not changed", () => {
Expand Down