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

Merged
merged 8 commits into from Oct 30, 2018
@@ -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: []
}
```

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

@@ -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: []
}
```

@@ -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).

This comment has been minimized.

Copy link
@not-an-aardvark

not-an-aardvark Oct 19, 2018

Member

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).

@@ -272,6 +272,28 @@ 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) {
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 });

This comment has been minimized.

Copy link
@not-an-aardvark

not-an-aardvark Oct 19, 2018

Member

Will this work correctly if a rule doesn't have a meta.docs property (e.g. a rule from a plugin)?

More generally, I'm wondering if we should move meta.docs.replacedBy to just meta.replacedBy, since the replacedBy field doesn't really relate to documentation in this context. (I know it's currently meta.docs.replacedBy on existing core rules, but it's easier to change that now than later since it's not a public API yet.)

edit: In either case, I think we should also update the Working with Rules page to note the new available options in the rule API.

This comment has been minimized.

Copy link
@calling

calling Oct 19, 2018

Author Contributor

That's a great point about meta.docs not necessarily existing. The "Working with Rules" page explicitly says that meta.docs is optional for custom rules and plugins: "In a custom rule or plugin, you can omit docs or include any properties that you need in it".

I'm happy to move replacedBy up to meta or I can add a property check (and document it either way), but I think where replacedBy should hang is best decided by the maintainers?

This comment has been minimized.

Copy link
@not-an-aardvark

not-an-aardvark Oct 23, 2018

Member

My personal preference would be to move it to meta.replacedBy rather than meta.docs.replacedBy, but let's see what other team members think.

cc @kaicataldo @platinumazure

This comment has been minimized.

Copy link
@kaicataldo

kaicataldo Oct 28, 2018

Member

Moving replacedBy to meta.replacedBy makes sense to me 👍

This comment has been minimized.

Copy link
@not-an-aardvark

not-an-aardvark Oct 19, 2018

Member

I think we should only include the replacedBy key if it's a string, and omit it otherwise. If a rule from a plugin does something weird and uses a different type of object as a replacedBy value, it could have the effect of crashing an integration/formatter.

}
});
}

return ruleWarnings;
}

/**
* Checks if the given message is an error message.
@@ -555,14 +577,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
};
}

@@ -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", () => {
@@ -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", () => {
@@ -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", () => {
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.