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

New: noInlineConfig setting (refs eslint/rfcs#22) #12091

Merged
merged 5 commits into from Aug 18, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions conf/config-schema.js
Expand Up @@ -20,6 +20,7 @@ const baseConfigProperties = {
processor: { type: "string" },
rules: { type: "object" },
settings: { type: "object" },
noInlineConfig: { type: "boolean" },

ecmaFeatures: { type: "object" } // deprecated; logs a warning when used
};
Expand Down
2 changes: 2 additions & 0 deletions lib/cli-engine/config-array-factory.js
Expand Up @@ -526,6 +526,7 @@ class ConfigArrayFactory {
env,
extends: extend,
globals,
noInlineConfig,
parser: parserName,
parserOptions,
plugins: pluginList,
Expand Down Expand Up @@ -567,6 +568,7 @@ class ConfigArrayFactory {
criteria: null,
env,
globals,
noInlineConfig,
parser,
parserOptions,
plugins,
Expand Down
7 changes: 7 additions & 0 deletions lib/cli-engine/config-array/config-array.js
Expand Up @@ -54,6 +54,7 @@ const { ExtractedConfig } = require("./extracted-config");
* @property {InstanceType<OverrideTester>|null} criteria The tester for the `files` and `excludedFiles` of this config element.
* @property {Record<string, boolean>|undefined} env The environment settings.
* @property {Record<string, GlobalConf>|undefined} globals The global variable settings.
* @property {boolean|undefined} noInlineConfig The flag that disables directive comments.
* @property {DependentParser|undefined} parser The parser loader.
* @property {Object|undefined} parserOptions The parser options.
* @property {Record<string, DependentPlugin>|undefined} plugins The plugin loaders.
Expand Down Expand Up @@ -250,6 +251,12 @@ function createConfig(instance, indices) {
config.processor = element.processor;
}

// Adopt the noInlineConfig which was found at first.
if (config.noInlineConfig === void 0 && element.noInlineConfig !== void 0) {
config.noInlineConfig = element.noInlineConfig;
config.configNameOfNoInlineConfig = element.name;
}

// Merge others.
mergeWithoutOverwrite(config.env, element.env);
mergeWithoutOverwrite(config.globals, element.globals);
Expand Down
17 changes: 16 additions & 1 deletion lib/cli-engine/config-array/extracted-config.js
Expand Up @@ -29,6 +29,12 @@
class ExtractedConfig {
constructor() {

/**
* The config name what `noInlineConfig` setting came from.
* @type {string}
*/
this.configNameOfNoInlineConfig = "";

/**
* Environments.
* @type {Record<string, boolean>}
Expand All @@ -41,6 +47,12 @@ class ExtractedConfig {
*/
this.globals = {};

/**
* The flag that disables directive comments.
* @type {boolean|undefined}
*/
this.noInlineConfig = void 0;

/**
* Parser definition.
* @type {DependentParser|null}
Expand Down Expand Up @@ -84,7 +96,10 @@ class ExtractedConfig {
*/
toCompatibleObjectAsConfigFileContent() {
const {
processor: _ignore, // eslint-disable-line no-unused-vars
/* eslint-disable no-unused-vars */
configNameOfNoInlineConfig: _ignore1,
processor: _ignore2,
/* eslint-enable no-unused-vars */
...config
} = this;

Expand Down
63 changes: 48 additions & 15 deletions lib/linter/linter.js
Expand Up @@ -198,14 +198,20 @@ function createMissingRuleMessage(ruleId) {
/**
* creates a linting problem
* @param {Object} options to create linting error
* @param {string} options.ruleId the ruleId to report
* @param {Object} options.loc the loc to report
* @param {string} options.message the error message to report
* @returns {Problem} created problem, returns a missing-rule problem if only provided ruleId.
* @param {string} [options.ruleId] the ruleId to report
* @param {Object} [options.loc] the loc to report
* @param {string} [options.message] the error message to report
* @param {string} [options.severity] the error message to report
* @returns {LintMessage} created problem, returns a missing-rule problem if only provided ruleId.
* @private
*/
function createLintingProblem(options) {
const { ruleId, loc = DEFAULT_ERROR_LOC, message = createMissingRuleMessage(options.ruleId) } = options;
const {
ruleId = null,
loc = DEFAULT_ERROR_LOC,
message = createMissingRuleMessage(options.ruleId),
severity = 2
} = options;

return {
ruleId,
Expand All @@ -214,7 +220,7 @@ function createLintingProblem(options) {
column: loc.start.column + 1,
endLine: loc.end.line,
endColumn: loc.end.column + 1,
severity: 2,
severity,
nodeType: null
};
}
Expand Down Expand Up @@ -257,10 +263,11 @@ function createDisableDirectives(options) {
* @param {string} filename The file being checked.
* @param {ASTNode} ast The top node of the AST.
* @param {function(string): {create: Function}} ruleMapper A map from rule IDs to defined rules
* @param {string|null} warnInlineConfig If a string then it should warn directive comments as disabled. The string value is the config name what the setting came from.
* @returns {{configuredRules: Object, enabledGlobals: {value:string,comment:Token}[], exportedVariables: Object, problems: Problem[], disableDirectives: DisableDirective[]}}
* A collection of the directive comments that were found, along with any problems that occurred when parsing
*/
function getDirectiveComments(filename, ast, ruleMapper) {
function getDirectiveComments(filename, ast, ruleMapper, warnInlineConfig) {
const configuredRules = {};
const enabledGlobals = Object.create(null);
const exportedVariables = {};
Expand All @@ -269,16 +276,29 @@ function getDirectiveComments(filename, ast, ruleMapper) {

ast.comments.filter(token => token.type !== "Shebang").forEach(comment => {
const trimmedCommentText = comment.value.trim();
const match = /^(eslint(-\w+){0,3}|exported|globals?)(\s|$)/u.exec(trimmedCommentText);
const match = /^(eslint(?:-env|-enable|-disable(?:(?:-next)?-line)?)?|exported|globals?)(?:\s|$)/u.exec(trimmedCommentText);

if (!match) {
return;
}
const lineCommentSupported = /^eslint-disable-(next-)?line$/u.test(match[1]);

if (warnInlineConfig && (lineCommentSupported || comment.type === "Block")) {
const kind = comment.type === "Block" ? `/*${match[1]}*/` : `//${match[1]}`;

problems.push(createLintingProblem({
ruleId: null,
message: `'${kind}' has no effect because you have 'noInlineConfig' setting in ${warnInlineConfig}.`,
loc: comment.loc,
severity: 1
}));
return;
}

const directiveValue = trimmedCommentText.slice(match.index + match[1].length);
let directiveType = "";

if (/^eslint-disable-(next-)?line$/u.test(match[1])) {
if (lineCommentSupported) {
if (comment.loc.start.line === comment.loc.end.line) {
directiveType = match[1].slice("eslint-".length);
} else {
Expand Down Expand Up @@ -441,16 +461,27 @@ function normalizeFilename(filename) {
return index === -1 ? filename : parts.slice(index).join(path.sep);
}

// eslint-disable-next-line valid-jsdoc
/**
* Normalizes the possible options for `linter.verify` and `linter.verifyAndFix` to a
* consistent shape.
* @param {VerifyOptions} providedOptions Options
* @returns {Required<VerifyOptions>} Normalized options
* @param {ConfigData} config Config.
* @returns {Required<VerifyOptions> & { warnInlineConfig: string|null }} Normalized options
*/
function normalizeVerifyOptions(providedOptions) {
function normalizeVerifyOptions(providedOptions, config) {
const disableInlineConfig = config.noInlineConfig === true;
const ignoreInlineConfig = providedOptions.allowInlineConfig === false;
const configNameOfNoInlineConfig = config.configNameOfNoInlineConfig
? ` (${config.configNameOfNoInlineConfig})`
: "";

return {
filename: normalizeFilename(providedOptions.filename || "<input>"),
allowInlineConfig: providedOptions.allowInlineConfig !== false,
allowInlineConfig: !ignoreInlineConfig,
warnInlineConfig: disableInlineConfig && !ignoreInlineConfig
? `your config${configNameOfNoInlineConfig}`
: null,
reportUnusedDisableDirectives: Boolean(providedOptions.reportUnusedDisableDirectives),
disableFixes: Boolean(providedOptions.disableFixes)
};
Expand Down Expand Up @@ -984,7 +1015,7 @@ class Linter {
_verifyWithoutProcessors(textOrSourceCode, providedConfig, providedOptions) {
const slots = internalSlotsMap.get(this);
const config = providedConfig || {};
const options = normalizeVerifyOptions(providedOptions);
const options = normalizeVerifyOptions(providedOptions, config);
let text;

// evaluate arguments
Expand Down Expand Up @@ -1019,7 +1050,9 @@ class Linter {
}

// search and apply "eslint-env *".
const envInFile = findEslintEnv(text);
const envInFile = options.allowInlineConfig && !options.warnInlineConfig
Copy link
Member

Choose a reason for hiding this comment

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

Nice find 👍

? findEslintEnv(text)
: {};
const resolvedEnvConfig = Object.assign({ builtin: true }, config.env, envInFile);
const enabledEnvs = Object.keys(resolvedEnvConfig)
.filter(envName => resolvedEnvConfig[envName])
Expand Down Expand Up @@ -1062,7 +1095,7 @@ class Linter {

const sourceCode = slots.lastSourceCode;
const commentDirectives = options.allowInlineConfig
? getDirectiveComments(options.filename, sourceCode.ast, ruleId => getRule(slots, ruleId))
? getDirectiveComments(options.filename, sourceCode.ast, ruleId => getRule(slots, ruleId), options.warnInlineConfig)
: { configuredRules: {}, enabledGlobals: {}, exportedVariables: {}, problems: [], disableDirectives: [] };

// augment global scope with declared global variables
Expand Down
2 changes: 2 additions & 0 deletions lib/shared/types.js
Expand Up @@ -30,6 +30,7 @@ module.exports = {};
* @property {Record<string, boolean>} [env] The environment settings.
* @property {string | string[]} [extends] The path to other config files or the package name of shareable configs.
* @property {Record<string, GlobalConf>} [globals] The global variable settings.
* @property {boolean} [noInlineConfig] The flag that disables directive comments.
* @property {OverrideConfigData[]} [overrides] The override settings per kind of files.
* @property {string} [parser] The path to a parser or the package name of a parser.
* @property {ParserOptions} [parserOptions] The parser options.
Expand All @@ -47,6 +48,7 @@ module.exports = {};
* @property {string | string[]} [extends] The path to other config files or the package name of shareable configs.
* @property {string | string[]} files The glob pattarns for target files.
* @property {Record<string, GlobalConf>} [globals] The global variable settings.
* @property {boolean} [noInlineConfig] The flag that disables directive comments.
* @property {OverrideConfigData[]} [overrides] The override settings per kind of files.
* @property {string} [parser] The path to a parser or the package name of a parser.
* @property {ParserOptions} [parserOptions] The parser options.
Expand Down
39 changes: 39 additions & 0 deletions tests/lib/cli-engine/cli-engine.js
Expand Up @@ -3495,6 +3495,45 @@ describe("CLIEngine", () => {
assert.deepStrictEqual(filenames, ["a.js", "b.js"]);
});
});

describe("with 'noInlineConfig' setting", () => {
const root = getFixturePath("cli-engine/noInlineConfig");

it("should warn directive comments if 'noInlineConfig' was given.", () => {
CLIEngine = defineCLIEngineWithInMemoryFileSystem({
cwd: () => root,
files: {
"test.js": "/* globals foo */",
".eslintrc.yml": "noInlineConfig: true"
}
}).CLIEngine;
engine = new CLIEngine();

const { results } = engine.executeOnFiles(["test.js"]);
const messages = results[0].messages;

assert.strictEqual(messages.length, 1);
assert.strictEqual(messages[0].message, "'/*globals*/' has no effect because you have 'noInlineConfig' setting in your config (.eslintrc.yml).");
});

it("should show the config file what the 'noInlineConfig' came from.", () => {
CLIEngine = defineCLIEngineWithInMemoryFileSystem({
cwd: () => root,
files: {
"node_modules/eslint-config-foo/index.js": "module.exports = {noInlineConfig: true}",
"test.js": "/* globals foo */",
".eslintrc.yml": "extends: foo"
}
}).CLIEngine;
engine = new CLIEngine();

const { results } = engine.executeOnFiles(["test.js"]);
const messages = results[0].messages;

assert.strictEqual(messages.length, 1);
assert.strictEqual(messages[0].message, "'/*globals*/' has no effect because you have 'noInlineConfig' setting in your config (.eslintrc.yml » eslint-config-foo).");
});
});
});

describe("getConfigForFile", () => {
Expand Down
2 changes: 2 additions & 0 deletions tests/lib/cli-engine/config-array-factory.js
Expand Up @@ -30,6 +30,7 @@ function assertConfigArrayElement(actual, providedExpected) {
criteria: null,
env: void 0,
globals: void 0,
noInlineConfig: void 0,
parser: void 0,
parserOptions: void 0,
plugins: void 0,
Expand All @@ -53,6 +54,7 @@ function assertConfig(actual, providedExpected) {
const expected = {
env: {},
globals: {},
noInlineConfig: void 0,
parser: null,
parserOptions: {},
plugins: [],
Expand Down
8 changes: 7 additions & 1 deletion tests/lib/cli-engine/config-array/config-array.js
Expand Up @@ -423,8 +423,10 @@ describe("ConfigArray", () => {
const result = merge(config[0], config[1]);

assert.deepStrictEqual(result, {
configNameOfNoInlineConfig: "",
env: {},
globals: {},
noInlineConfig: void 0,
parser: null,
parserOptions: {
ecmaFeatures: {
Expand Down Expand Up @@ -452,8 +454,10 @@ describe("ConfigArray", () => {
const result = merge(config[0], config[1]);

assert.deepStrictEqual(result, {
configNameOfNoInlineConfig: "",
env: {},
globals: {},
noInlineConfig: void 0,
parser: null,
parserOptions: {
ecmaFeatures: {
Expand Down Expand Up @@ -565,6 +569,7 @@ describe("ConfigArray", () => {
const result = merge(config[0], config[1]);

assert.deepStrictEqual(result, {
configNameOfNoInlineConfig: "",
parser: null,
parserOptions: {
ecmaFeatures: {
Expand Down Expand Up @@ -601,7 +606,8 @@ describe("ConfigArray", () => {
"valid-jsdoc": [2]
},
settings: {},
processor: null
processor: null,
noInlineConfig: void 0
});
assert.deepStrictEqual(config[0], {
rules: {
Expand Down