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

WIP: Validate rule options #2179

Closed
wants to merge 12 commits into from
  •  
  •  
  •  
35 changes: 34 additions & 1 deletion docs/developer-guide/working-with-rules.md
Expand Up @@ -21,6 +21,10 @@ module.exports = function(context) {
};

};

module.exports.schema = [
// JSON Schema for rule options goes here
];
```

**Important:** Rule submissions will not be accepted unless they are in this format.
Expand Down Expand Up @@ -132,6 +136,34 @@ Since `context.options` is just an array, you can use it to determine how many o

When using options, make sure that your rule has some logic defaults in case the options are not provided.

### Options Schemas

Rules may export a `schema` property, which is a [JSON schema](http://json-schema.org/) format description of a rule's options which will be used by ESLint to validate configuration options and prevent invalid or unexpected inputs before they are passed to the rule in `context.options`.

There are two formats for a rule's exported `schema`. The first is a full JSON Schema object describing all possible options the rule accepts, including the rule's error level as the first argument and any optional arguments thereafter.

However, to simplify schema creation, rules may also export an array of schemas for each optional positional argument, and ESLint will automatically validate the required error level first. For example, the `yoda` rule accepts a primary mode argument, as well as an extra options object with named properties.

```js
// "yoda": [2, "never", { "exceptRange": true }]
module.exports.schema = [
{
"enum": ["always", "never"]
},
{
"type": "object",
"properties": {
"exceptRange": {
"type": "boolean"
}
},
"additionalProperties": false
}
];
```

In the preceding example, the error level is assumed to be the first argument. It is followed by the first optional argument, a string which may be either `"always"` or `"never"`. The final optional argument is an object, which may have a Boolean property named `exceptRange`.

### Getting the Source

If your rule needs to get the actual JavaScript source to work with, then use the `context.getSource()` method. This method works as follows:
Expand Down Expand Up @@ -191,13 +223,14 @@ The basic pattern for a rule unit test file is:
//------------------------------------------------------------------------------

var eslint = require("../../../lib/eslint"),
validate = require("../../../lib/validate-options"),
ESLintTester = require("eslint-tester");

//------------------------------------------------------------------------------
// Tests
//------------------------------------------------------------------------------

var eslintTester = new ESLintTester(eslint);
var eslintTester = new ESLintTester(eslint, validate);
eslintTester.addRuleTest("lib/rules/block-scoped-var", {

// Examples of code that should not trigger the rule
Expand Down
7 changes: 6 additions & 1 deletion lib/cli-engine.js
Expand Up @@ -27,7 +27,8 @@ var fs = require("fs"),
traverse = require("./util/traverse"),
IgnoredPaths = require("./ignored-paths"),
Config = require("./config"),
util = require("./util");
util = require("./util"),
validate = require("./validate-options");

//------------------------------------------------------------------------------
// Typedefs
Expand Down Expand Up @@ -285,6 +286,10 @@ function CLIEngine(options) {
rules.load(rulesdir);
});
}

Object.keys(this.options.rules || {}).forEach(function(name) {
validate(name, this.options.rules[name], "CLI");
}.bind(this));
}

CLIEngine.prototype = {
Expand Down
21 changes: 14 additions & 7 deletions lib/config.js
Expand Up @@ -11,17 +11,18 @@
// Requirements
//------------------------------------------------------------------------------

var fs = require("fs"),
path = require("path"),
var assign = require("object-assign"),
debug = require("debug"),
environments = require("../conf/environments"),
util = require("./util"),
FileFinder = require("./file-finder"),
fs = require("fs"),
isAbsolutePath = require("path-is-absolute"),
path = require("path"),
stripComments = require("strip-json-comments"),
assign = require("object-assign"),
debug = require("debug"),
yaml = require("js-yaml"),
userHome = require("user-home"),
isAbsolutePath = require("path-is-absolute");
util = require("./util"),
validate = require("./validate-options"),
yaml = require("js-yaml");

//------------------------------------------------------------------------------
// Constants
Expand Down Expand Up @@ -86,6 +87,12 @@ function loadConfig(filePath) {
config = require(filePath);
}

if (config.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 have to debug through the code to be sure, but it feels like you are going to run validations multiple times on the same rule. Once here, and once in eslint.js, and if eslint called programmatically, an extra time in CLI-engine.

Copy link
Member Author

Choose a reason for hiding this comment

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

Since configs are just simple objects full of name/options pairs, options have to be validated when location information is still available. My first approach was to validate exactly once in verify when the final configuration had been determined, but we wanted to be able to print the source of any invalid options, so I moved validation out into the locations you listed.

I suppose the alternative approach would be to attach source location metadata to config objects, but that seemed like overkill.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, attaching metadata seems like an overkill.

Object.keys(config.rules).forEach(function (name) {
validate(name, config.rules[name], filePath);
});
}

// If an `extends` property is defined, it represents a configuration file to use as
// a "parent". Load the referenced file and merge the configuration recursively.
if (config.extends) {
Expand Down
23 changes: 13 additions & 10 deletions lib/eslint.js
Expand Up @@ -8,17 +8,18 @@
// Requirements
//------------------------------------------------------------------------------

var estraverse = require("estraverse-fb"),
escope = require("escope"),
var assign = require("object-assign"),
createTokenStore = require("./token-store.js"),
environments = require("../conf/environments"),
assign = require("object-assign"),
rules = require("./rules"),
util = require("./util"),
escapeRegExp = require("escape-string-regexp"),
escope = require("escope"),
estraverse = require("estraverse-fb"),
EventEmitter = require("events").EventEmitter,
RuleContext = require("./rule-context"),
rules = require("./rules"),
timing = require("./timing"),
createTokenStore = require("./token-store.js"),
EventEmitter = require("events").EventEmitter,
escapeRegExp = require("escape-string-regexp");
util = require("./util"),
validate = require("./validate-options");

//------------------------------------------------------------------------------
// Helpers
Expand Down Expand Up @@ -238,13 +239,14 @@ function enableReporting(reportingConfig, start, rulesToEnable) {
* Parses comments in file to extract file-specific config of rules, globals
* and environments and merges them with global config; also code blocks
* where reporting is disabled or enabled and merges them with reporting config.
* @param {string} filename The file being checked.
* @param {ASTNode} ast The top node of the AST.
* @param {Object} config The existing configuration data.
* @param {Object[]} reportingConfig The existing reporting configuration data.
* @param {Object[]} messages The messages queue.
* @returns {void}
*/
function modifyConfigsFromComments(ast, config, reportingConfig, messages) {
function modifyConfigsFromComments(filename, ast, config, reportingConfig, messages) {

var commentConfig = {
astGlobals: {},
Expand Down Expand Up @@ -285,6 +287,7 @@ function modifyConfigsFromComments(ast, config, reportingConfig, messages) {
Object.keys(items).forEach(function(name) {
var ruleValue = items[name];
if (typeof ruleValue === "number" || (Array.isArray(ruleValue) && typeof ruleValue[0] === "number")) {
validate(name, ruleValue, filename + " line " + comment.loc.start.line);
commentRules[name] = ruleValue;
}
});
Expand Down Expand Up @@ -613,7 +616,7 @@ module.exports = (function() {
currentAST = ast;

// parse global comments and modify config
modifyConfigsFromComments(ast, config, reportingConfig, messages);
modifyConfigsFromComments(filename, ast, config, reportingConfig, messages);

// enable appropriate rules
Object.keys(config.rules).filter(function(key) {
Expand Down
2 changes: 2 additions & 0 deletions lib/rules/block-scoped-var.js
Expand Up @@ -316,3 +316,5 @@ module.exports = function(context) {
};

};

module.exports.schema = [];
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be easier to just check if there's a schema, then validate, otherwise skip?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, that's exactly what it does if there's no schema. In this case, the schema specifies that block-scoped-var does not accept any options, so if any are supplied, it will warn that there are too many.

Copy link
Member

Choose a reason for hiding this comment

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

Ahh.. ok, got it. Thanks.

15 changes: 15 additions & 0 deletions lib/rules/brace-style.js
Expand Up @@ -211,3 +211,18 @@ module.exports = function(context) {
};

};

module.exports.schema = [
{
"enum": ["1tbs", "stroustrup"]
},
{
"type": "object",
"properties": {
"allowSingleLine": {
"type": "boolean"
}
},
"additionalProperties": false
}
];
12 changes: 12 additions & 0 deletions lib/rules/camelcase.js
Expand Up @@ -97,3 +97,15 @@ module.exports = function(context) {
};

};

module.exports.schema = [
{
"type": "object",
"properties": {
"properties": {
"enum": ["always", "never"]
}
},
"additionalProperties": false
}
];
6 changes: 6 additions & 0 deletions lib/rules/comma-dangle.js
Expand Up @@ -56,3 +56,9 @@ module.exports = function (context) {
"ArrayExpression": checkForTrailingComma
};
};

module.exports.schema = [
{
"enum": ["always", "always-multiline", "never"]
}
];
15 changes: 15 additions & 0 deletions lib/rules/comma-spacing.js
Expand Up @@ -157,3 +157,18 @@ module.exports = function(context) {
};

};

module.exports.schema = [
{
"type": "object",
"properties": {
"before": {
"type": "boolean"
},
"after": {
"type": "boolean"
}
},
"additionalProperties": false
}
];
18 changes: 18 additions & 0 deletions lib/rules/comma-style.js
Expand Up @@ -175,3 +175,21 @@ module.exports = function(context) {

return nodes;
};

module.exports.schema = [
{
"enum": ["first", "last"]
},
{
"type": "object",
"properties": {
"exceptions": {
"type": "object",
"additionalProperties": {
"type": "boolean"
}
}
},
"additionalProperties": false
}
];
6 changes: 6 additions & 0 deletions lib/rules/complexity.js
Expand Up @@ -86,3 +86,9 @@ module.exports = function(context) {
};

};

module.exports.schema = [
{
"type": "integer"
}
];
2 changes: 2 additions & 0 deletions lib/rules/consistent-return.js
Expand Up @@ -71,3 +71,5 @@ module.exports = function(context) {
};

};

module.exports.schema = [];
6 changes: 6 additions & 0 deletions lib/rules/consistent-this.js
Expand Up @@ -106,3 +106,9 @@ module.exports = function(context) {
};

};

module.exports.schema = [
{
"type": "string"
}
];
6 changes: 6 additions & 0 deletions lib/rules/curly.js
Expand Up @@ -101,3 +101,9 @@ module.exports = function(context) {
};

};

module.exports.schema = [
{
"enum": ["all", "multi", "multi-line"]
}
];
2 changes: 2 additions & 0 deletions lib/rules/default-case.js
Expand Up @@ -62,3 +62,5 @@ module.exports = function(context) {
}
};
};

module.exports.schema = [];
15 changes: 15 additions & 0 deletions lib/rules/dot-notation.js
Expand Up @@ -102,3 +102,18 @@ module.exports = function(context) {
}
};
};

module.exports.schema = [
{
"type": "object",
"properties": {
"allowKeywords": {
"type": "boolean"
},
"allowPattern": {
"type": "string"
}
},
"additionalProperties": false
}
];
2 changes: 2 additions & 0 deletions lib/rules/eol-last.js
Expand Up @@ -34,3 +34,5 @@ module.exports = function(context) {
};

};

module.exports.schema = [];
6 changes: 6 additions & 0 deletions lib/rules/eqeqeq.js
Expand Up @@ -88,3 +88,9 @@ module.exports = function(context) {
};

};

module.exports.schema = [
{
"enum": ["smart", "allow-null"]
}
];
2 changes: 2 additions & 0 deletions lib/rules/func-names.js
Expand Up @@ -41,3 +41,5 @@ module.exports = function(context) {
}
};
};

module.exports.schema = [];
6 changes: 6 additions & 0 deletions lib/rules/func-style.js
Expand Up @@ -41,3 +41,9 @@ module.exports = function(context) {
};

};

module.exports.schema = [
{
"enum": ["declaration", "expression"]
}
];
6 changes: 6 additions & 0 deletions lib/rules/generator-star-spacing.js
Expand Up @@ -79,3 +79,9 @@ module.exports = function(context) {
};

};

module.exports.schema = [
{
"enum": ["before", "after", "both", "neither"]
}
];
6 changes: 6 additions & 0 deletions lib/rules/generator-star.js
Expand Up @@ -68,3 +68,9 @@ module.exports = function(context) {
};

};

module.exports.schema = [
{
"enum": ["start", "middle", "end"]
}
];
6 changes: 6 additions & 0 deletions lib/rules/global-strict.js
Expand Up @@ -41,3 +41,9 @@ module.exports = function(context) {
}

};

module.exports.schema = [
{
"enum": ["always", "never"]
}
];