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 2 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
6 changes: 6 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,11 @@ 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;
}

// Merge others.
mergeWithoutOverwrite(config.env, element.env);
mergeWithoutOverwrite(config.globals, element.globals);
Expand Down
6 changes: 6 additions & 0 deletions lib/cli-engine/config-array/extracted-config.js
Expand Up @@ -41,6 +41,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
53 changes: 39 additions & 14 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 {boolean} warnInlineConfig If `true` then warns directive comments.
* @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,12 +276,22 @@ 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;
}

if (warnInlineConfig) {
problems.push(createLintingProblem({
ruleId: null,
message: `'/*${match[1]}*/' has no effect because you have 'noInlineConfig' setting in your config.`,
loc: comment.loc,
severity: 1
}));
return;
}

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

Expand Down Expand Up @@ -441,16 +458,22 @@ 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: boolean }} Normalized options
*/
function normalizeVerifyOptions(providedOptions) {
function normalizeVerifyOptions(providedOptions, config) {
const disableInlineConfig = config.noInlineConfig === true;
const ignoreInlineConfig = providedOptions.allowInlineConfig === false;

return {
filename: normalizeFilename(providedOptions.filename || "<input>"),
allowInlineConfig: providedOptions.allowInlineConfig !== false,
allowInlineConfig: !ignoreInlineConfig,
warnInlineConfig: disableInlineConfig && !ignoreInlineConfig,
reportUnusedDisableDirectives: Boolean(providedOptions.reportUnusedDisableDirectives),
disableFixes: Boolean(providedOptions.disableFixes)
};
Expand Down Expand Up @@ -984,7 +1007,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 +1042,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, options.warnInlineConfig)
: {};
const resolvedEnvConfig = Object.assign({ builtin: true }, config.env, envInFile);
const enabledEnvs = Object.keys(resolvedEnvConfig)
.filter(envName => resolvedEnvConfig[envName])
Expand Down Expand Up @@ -1062,7 +1087,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
21 changes: 21 additions & 0 deletions tests/lib/cli-engine/cli-engine.js
Expand Up @@ -3495,6 +3495,27 @@ 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 effects because you have 'noInlineConfig' setting in your config.");
});
});
});

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
5 changes: 4 additions & 1 deletion tests/lib/cli-engine/config-array/config-array.js
Expand Up @@ -425,6 +425,7 @@ describe("ConfigArray", () => {
assert.deepStrictEqual(result, {
env: {},
globals: {},
noInlineConfig: void 0,
parser: null,
parserOptions: {
ecmaFeatures: {
Expand Down Expand Up @@ -454,6 +455,7 @@ describe("ConfigArray", () => {
assert.deepStrictEqual(result, {
env: {},
globals: {},
noInlineConfig: void 0,
parser: null,
parserOptions: {
ecmaFeatures: {
Expand Down Expand Up @@ -601,7 +603,8 @@ describe("ConfigArray", () => {
"valid-jsdoc": [2]
},
settings: {},
processor: null
processor: null,
noInlineConfig: void 0
});
assert.deepStrictEqual(config[0], {
rules: {
Expand Down
57 changes: 34 additions & 23 deletions tests/lib/linter/linter.js
Expand Up @@ -74,7 +74,7 @@ const ESLINT_ENV = "eslint-env";
describe("Linter", () => {
const filename = "filename.js";

/** @type {InstanceType<import("../../lib/linter.js")["Linter"]>} */
/** @type {InstanceType<import("../../../lib/linter/linter.js")["Linter"]>} */
let linter;

beforeEach(() => {
Expand Down Expand Up @@ -3134,37 +3134,48 @@ describe("Linter", () => {

it("should report a violation for env changes", () => {
const code = [
`/*${ESLINT_ENV} browser*/`
`/*${ESLINT_ENV} browser*/ window`
].join("\n");
const config = {
rules: {
test: 2
"no-undef": 2
}
};
let ok = false;

linter.defineRules({
test(context) {
return {
Program() {
const scope = context.getScope();
const sourceCode = context.getSourceCode();
const comments = sourceCode.getAllComments();

assert.strictEqual(1, comments.length);

const windowVar = getVariable(scope, "window");
const messages = linter.verify(code, config, { allowInlineConfig: false });

assert.notOk(windowVar.eslintExplicitGlobal);
assert.strictEqual(messages.length, 1);
assert.strictEqual(messages[0].ruleId, "no-undef");
});
});

ok = true;
}
};
}
describe("when evaluating code with 'noInlineComment'", () => {
for (const directive of [
"globals foo",
"global foo",
"exported foo",
"eslint eqeqeq: error",
"eslint-disable eqeqeq",
"eslint-disable-line eqeqeq",
"eslint-disable-next-line eqeqeq",
"eslint-enable eqeqeq",
"eslint-env es6"
mysticatea marked this conversation as resolved.
Show resolved Hide resolved
]) {
// eslint-disable-next-line no-loop-func
Copy link
Member

Choose a reason for hiding this comment

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

Just out of curiosity, why does no-loop-func think there is a problem here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably due to linter variable. It's shared in the callback in this loop.

it(`should warn '/* ${directive} */' if 'noInlineConfig' was given.`, () => {
const messages = linter.verify(`/* ${directive} */`, { noInlineConfig: true });

assert.deepStrictEqual(messages.length, 1);
assert.deepStrictEqual(messages[0].fatal, void 0);
assert.deepStrictEqual(messages[0].ruleId, null);
assert.deepStrictEqual(messages[0].severity, 1);
assert.deepStrictEqual(messages[0].message, `'/*${directive.split(" ")[0]}*/' has no effects because you have 'noInlineConfig' setting in your config.`);
});
}

linter.verify(code, config, { allowInlineConfig: false });
assert(ok);
it("should not warn if 'noInlineConfig' and '--no-inline-config' were given.", () => {
const messages = linter.verify("/* globals foo */", { noInlineConfig: true }, { allowInlineConfig: false });

assert.deepStrictEqual(messages.length, 0);
});
});

Expand Down