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

Chore: remove undocumented Linter#rules property (refs #9161) #11335

Merged
merged 3 commits into from Jan 30, 2019
Merged

Chore: remove undocumented Linter#rules property (refs #9161) #11335

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1676355
Chore: remove undocumented `Linter.rules` property (refs #9161)
not-an-aardvark Jan 30, 2019
7eaed27
Keep numbers in comments up to date
not-an-aardvark Jan 30, 2019
c5dc5e8
Add tests for loading rules from a directory
not-an-aardvark Jan 30, 2019
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
32 changes: 1 addition & 31 deletions Makefile.js
View file
Open in desktop
Expand Up @@ -117,27 +117,6 @@ function fileType(extension) {
};
}

/**
* Generates a static file that includes each rule by name rather than dynamically
* looking up based on directory. This is used for the browser version of ESLint.
* @param {string} basedir The directory in which to look for code.
* @returns {void}
*/
function generateRulesIndex(basedir) {
let output = "module.exports = function() {\n";

output += " var rules = Object.create(null);\n";

find(`${basedir}rules/`).filter(fileType("js")).forEach(filename => {
const basename = path.basename(filename, ".js");

output += ` rules["${basename}"] = require("./rules/${basename}");\n`;
});

output += "\n return rules;\n};";
output.to(`${basedir}load-rules.js`);
}

/**
* Executes a command and returns the output instead of printing it to stdout.
* @param {string} cmd The command string to execute.
Expand Down Expand Up @@ -841,17 +820,8 @@ target.browserify = function() {
mkdir(BUILD_DIR);
}

// 2. copy files into temp directory
cp("-r", "lib/*", TEMP_DIR);

// 3. delete the load-rules.js file
rm("-rf", `${TEMP_DIR}load-rules.js`);

// 4. create new load-rule.js with hardcoded requires
generateRulesIndex(TEMP_DIR);

// 5. browserify the temp directory
not-an-aardvark marked this conversation as resolved.
Show resolved Hide resolved
exec(`${getBinFile("browserify")} -x espree ${TEMP_DIR}linter.js -o ${BUILD_DIR}eslint.js -s eslint --global-transform [ babelify --presets [ es2015 ] ]`);
exec(`${getBinFile("browserify")} -x espree lib/linter.js -o ${BUILD_DIR}eslint.js -s eslint --global-transform [ babelify --presets [ es2015 ] ]`);

// 6. Browserify espree
exec(`${getBinFile("browserify")} -r espree -o ${TEMP_DIR}espree.js --global-transform [ babelify --presets [ es2015 ] ]`);
Expand Down
5 changes: 3 additions & 2 deletions lib/cli-engine.js
View file
Open in desktop
Expand Up @@ -29,7 +29,8 @@ const fs = require("fs"),
hash = require("./util/hash"),
ModuleResolver = require("./util/module-resolver"),
naming = require("./util/naming"),
pkg = require("../package.json");
pkg = require("../package.json"),
loadRules = require("./load-rules");

const debug = require("debug")("eslint:cli-engine");
const resolver = new ModuleResolver();
Expand Down Expand Up @@ -447,7 +448,7 @@ class CLIEngine {

this.options.rulePaths.forEach(rulesdir => {
debug(`Loading rules from ${rulesdir}`);
this.linter.rules.load(rulesdir, cwd);
this.linter.defineRules(loadRules(rulesdir, cwd));
});
}

Expand Down
2 changes: 1 addition & 1 deletion lib/config.js
View file
Open in desktop
Expand Up @@ -71,7 +71,7 @@ class Config {
const options = providedOptions || {};

this.linterContext = linterContext;
this.plugins = new Plugins(linterContext.environments, linterContext.rules);
this.plugins = new Plugins(linterContext.environments, linterContext.defineRule.bind(linterContext));

this.options = options;
this.ignore = options.ignore;
Expand Down
16 changes: 12 additions & 4 deletions lib/config/plugins.js
View file
Open in desktop
Expand Up @@ -24,12 +24,12 @@ class Plugins {
/**
* Creates the plugins context
* @param {Environments} envContext - env context
* @param {Rules} rulesContext - rules context
* @param {function(string, Rule): void} defineRule - Callback for when a plugin is defined which introduces rules
*/
constructor(envContext, rulesContext) {
constructor(envContext, defineRule) {
this._plugins = Object.create(null);
this._environments = envContext;
this._rules = rulesContext;
this._defineRule = defineRule;
}

/**
Expand All @@ -45,7 +45,15 @@ class Plugins {
// load up environments and rules
this._plugins[shortName] = plugin;
this._environments.importPlugin(plugin, shortName);
this._rules.importPlugin(plugin, shortName);

if (plugin.rules) {
Object.keys(plugin.rules).forEach(ruleId => {
const qualifiedRuleId = `${shortName}/${ruleId}`,
rule = plugin.rules[ruleId];

this._defineRule(qualifiedRuleId, rule);
});
}
}

/**
Expand Down
12 changes: 6 additions & 6 deletions lib/linter.js
View file
Open in desktop
Expand Up @@ -758,6 +758,7 @@ function runRules(sourceCode, configuredRules, ruleMapper, parserOptions, parser

const lastSourceCodes = new WeakMap();
const loadedParserMaps = new WeakMap();
const ruleMaps = new WeakMap();

//------------------------------------------------------------------------------
// Public Interface
Expand All @@ -772,9 +773,8 @@ module.exports = class Linter {
constructor() {
lastSourceCodes.set(this, null);
loadedParserMaps.set(this, new Map());
ruleMaps.set(this, new Rules());
this.version = pkg.version;

this.rules = new Rules();
this.environments = new Environments();
}

Expand Down Expand Up @@ -873,7 +873,7 @@ module.exports = class Linter {

const sourceCode = lastSourceCodes.get(this);
const commentDirectives = options.allowInlineConfig
? getDirectiveComments(options.filename, sourceCode.ast, ruleId => this.rules.get(ruleId))
? getDirectiveComments(options.filename, sourceCode.ast, ruleId => ruleMaps.get(this).get(ruleId))
: { configuredRules: {}, enabledGlobals: {}, exportedVariables: {}, problems: [], disableDirectives: [] };

// augment global scope with declared global variables
Expand All @@ -891,7 +891,7 @@ module.exports = class Linter {
lintingProblems = runRules(
sourceCode,
configuredRules,
ruleId => this.rules.get(ruleId),
ruleId => ruleMaps.get(this).get(ruleId),
parserOptions,
parserName,
settings,
Expand Down Expand Up @@ -957,7 +957,7 @@ module.exports = class Linter {
* @returns {void}
*/
defineRule(ruleId, ruleModule) {
this.rules.define(ruleId, ruleModule);
ruleMaps.get(this).define(ruleId, ruleModule);
}

/**
Expand All @@ -976,7 +976,7 @@ module.exports = class Linter {
* @returns {Map} All loaded rules
*/
getRules() {
return this.rules.getAllLoadedRules();
return ruleMaps.get(this).getAllLoadedRules();
}

/**
Expand Down
45 changes: 3 additions & 42 deletions lib/rules.js
View file
Open in desktop
Expand Up @@ -10,7 +10,6 @@
//------------------------------------------------------------------------------

const lodash = require("lodash");
const loadRules = require("./load-rules");
const ruleReplacements = require("../conf/replacements").rules;
const builtInRules = require("./built-in-rules-index");

Expand Down Expand Up @@ -60,7 +59,9 @@ function normalizeRule(rule) {
class Rules {
constructor() {
this._rules = Object.create(null);
this.defineAll(builtInRules);
Object.keys(builtInRules).forEach(ruleId => {
this.define(ruleId, builtInRules[ruleId]);
});
}

/**
Expand All @@ -73,46 +74,6 @@ class Rules {
this._rules[ruleId] = normalizeRule(ruleModule);
}

/**
* Loads and registers all rules from passed rules directory.
* @param {string} [rulesDir] Path to rules directory, may be relative. Defaults to `lib/rules`.
* @param {string} cwd Current working directory
* @returns {void}
*/
load(rulesDir, cwd) {
const newRules = loadRules(rulesDir, cwd);

this.defineAll(newRules);
}

/**
* Pulls a Map of new rules to the defined ones of this instance.
* @param {Object} newRules Expects to have an object here that maps the rule ID to the rule definition.
* @returns {void}
*/
defineAll(newRules) {
Object.keys(newRules).forEach(ruleId => {
this.define(ruleId, newRules[ruleId]);
});
}

/**
* Registers all given rules of a plugin.
* @param {Object} plugin The plugin object to import.
* @param {string} pluginName The name of the plugin without prefix (`eslint-plugin-`).
* @returns {void}
*/
importPlugin(plugin, pluginName) {
if (plugin.rules) {
Object.keys(plugin.rules).forEach(ruleId => {
const qualifiedRuleId = `${pluginName}/${ruleId}`,
rule = plugin.rules[ruleId];

this.define(qualifiedRuleId, rule);
});
}
}

/**
* Access rule handler by id (file name).
* @param {string} ruleId Rule id (file name).
Expand Down
23 changes: 12 additions & 11 deletions tests/lib/config/config-validator.js
View file
Open in desktop
Expand Up @@ -11,7 +11,8 @@

const assert = require("chai").assert,
Linter = require("../../../lib/linter"),
validator = require("../../../lib/config/config-validator");
validator = require("../../../lib/config/config-validator"),
Rules = require("../../../lib/rules");
const linter = new Linter();

//------------------------------------------------------------------------------
Expand Down Expand Up @@ -96,7 +97,7 @@ describe("Validator", () => {
* @returns {{create: Function}} The loaded rule
*/
function ruleMapper(ruleId) {
return linter.getRules().get(ruleId);
return linter.getRules().get(ruleId) || new Rules().get(ruleId);
}

beforeEach(() => {
Expand Down Expand Up @@ -403,12 +404,12 @@ describe("Validator", () => {
describe("getRuleOptionsSchema", () => {

it("should return null for a missing rule", () => {
assert.strictEqual(validator.getRuleOptionsSchema(linter.rules.get("non-existent-rule")), null);
assert.strictEqual(validator.getRuleOptionsSchema(ruleMapper("non-existent-rule")), null);
});

it("should not modify object schema", () => {
linter.defineRule("mock-object-rule", mockObjectRule);
assert.deepStrictEqual(validator.getRuleOptionsSchema(linter.rules.get("mock-object-rule")), {
assert.deepStrictEqual(validator.getRuleOptionsSchema(ruleMapper("mock-object-rule")), {
enum: ["first", "second"]
});
});
Expand All @@ -418,43 +419,43 @@ describe("Validator", () => {
describe("validateRuleOptions", () => {

it("should throw for incorrect warning level number", () => {
const fn = validator.validateRuleOptions.bind(null, linter.rules.get("mock-rule"), "mock-rule", 3, "tests");
const fn = validator.validateRuleOptions.bind(null, ruleMapper("mock-rule"), "mock-rule", 3, "tests");

assert.throws(fn, "tests:\n\tConfiguration for rule \"mock-rule\" is invalid:\n\tSeverity should be one of the following: 0 = off, 1 = warn, 2 = error (you passed '3').\n");
});

it("should throw for incorrect warning level string", () => {
const fn = validator.validateRuleOptions.bind(null, linter.rules.get("mock-rule"), "mock-rule", "booya", "tests");
const fn = validator.validateRuleOptions.bind(null, ruleMapper("mock-rule"), "mock-rule", "booya", "tests");

assert.throws(fn, "tests:\n\tConfiguration for rule \"mock-rule\" is invalid:\n\tSeverity should be one of the following: 0 = off, 1 = warn, 2 = error (you passed '\"booya\"').\n");
});

it("should throw for invalid-type warning level", () => {
const fn = validator.validateRuleOptions.bind(null, linter.rules.get("mock-rule"), "mock-rule", [["error"]], "tests");
const fn = validator.validateRuleOptions.bind(null, ruleMapper("mock-rule"), "mock-rule", [["error"]], "tests");

assert.throws(fn, "tests:\n\tConfiguration for rule \"mock-rule\" is invalid:\n\tSeverity should be one of the following: 0 = off, 1 = warn, 2 = error (you passed '[ \"error\" ]').\n");
});

it("should only check warning level for nonexistent rules", () => {
const fn = validator.validateRuleOptions.bind(null, linter.rules.get("non-existent-rule"), "non-existent-rule", [3, "foobar"], "tests");
const fn = validator.validateRuleOptions.bind(null, ruleMapper("non-existent-rule"), "non-existent-rule", [3, "foobar"], "tests");

assert.throws(fn, "tests:\n\tConfiguration for rule \"non-existent-rule\" is invalid:\n\tSeverity should be one of the following: 0 = off, 1 = warn, 2 = error (you passed '3').\n");
});

it("should only check warning level for plugin rules", () => {
const fn = validator.validateRuleOptions.bind(null, linter.rules.get("plugin/rule"), "plugin/rule", 3, "tests");
const fn = validator.validateRuleOptions.bind(null, ruleMapper("plugin/rule"), "plugin/rule", 3, "tests");

assert.throws(fn, "tests:\n\tConfiguration for rule \"plugin/rule\" is invalid:\n\tSeverity should be one of the following: 0 = off, 1 = warn, 2 = error (you passed '3').\n");
});

it("should throw for incorrect configuration values", () => {
const fn = validator.validateRuleOptions.bind(null, linter.rules.get("mock-rule"), "mock-rule", [2, "frist"], "tests");
const fn = validator.validateRuleOptions.bind(null, ruleMapper("mock-rule"), "mock-rule", [2, "frist"], "tests");

assert.throws(fn, "tests:\n\tConfiguration for rule \"mock-rule\" is invalid:\n\tValue \"frist\" should be equal to one of the allowed values.\n");
});

it("should throw for too many configuration values", () => {
const fn = validator.validateRuleOptions.bind(null, linter.rules.get("mock-rule"), "mock-rule", [2, "first", "second"], "tests");
const fn = validator.validateRuleOptions.bind(null, ruleMapper("mock-rule"), "mock-rule", [2, "first", "second"], "tests");

assert.throws(fn, "tests:\n\tConfiguration for rule \"mock-rule\" is invalid:\n\tValue [\"first\",\"second\"] should NOT have more than 1 items.\n");
});
Expand Down
18 changes: 9 additions & 9 deletions tests/lib/config/plugins.js
View file
Open in desktop
Expand Up @@ -10,7 +10,6 @@

const assert = require("chai").assert,
Plugins = require("../../../lib/config/plugins"),
Rules = require("../../../lib/rules"),
Environments = require("../../../lib/config/environments");

const proxyquire = require("proxyquire").noCallThru().noPreserveCache();
Expand All @@ -24,7 +23,7 @@ describe("Plugins", () => {
describe("get", () => {

it("should return null when plugin doesn't exist", () => {
assert.isNull((new Plugins(new Environments(), new Rules())).get("foo"));
assert.isNull((new Plugins(new Environments(), () => {})).get("foo"));
});
});

Expand All @@ -39,8 +38,8 @@ describe("Plugins", () => {
beforeEach(() => {
plugin = {};
scopedPlugin = {};
rules = new Rules();
environments = new Environments();
rules = new Map();
StubbedPlugins = new (proxyquire("../../../lib/config/plugins", {
"eslint-plugin-example": plugin,
"@scope/eslint-plugin-example": scopedPlugin,
Expand All @@ -49,7 +48,7 @@ describe("Plugins", () => {
throw new Error("error thrown while loading this module");
}
}
}))(environments, rules);
}))(environments, rules.set.bind(rules));
});

it("should load a plugin when referenced by short name", () => {
Expand Down Expand Up @@ -180,7 +179,7 @@ describe("Plugins", () => {
};
StubbedPlugins.load("@scope/eslint-plugin-example");

assert.isFalse(rules.getAllLoadedRules().has("example/foo"));
assert.isFalse(rules.has("example/foo"));
});
});
});
Expand All @@ -189,17 +188,18 @@ describe("Plugins", () => {

let StubbedPlugins,
plugin1,
plugin2;
const rules = new Rules(),
environments = new Environments();
plugin2,
rules;
const environments = new Environments();

beforeEach(() => {
plugin1 = {};
plugin2 = {};
rules = new Map();
StubbedPlugins = new (proxyquire("../../../lib/config/plugins", {
"eslint-plugin-example1": plugin1,
"eslint-plugin-example2": plugin2
}))(environments, rules);
}))(environments, rules.set.bind(rules));
});

it("should load plugins when passed multiple plugins", () => {
Expand Down