From e736ba9e31acfa2dda5c25c8964b1ed36f514ddd Mon Sep 17 00:00:00 2001 From: Adi Dahiya Date: Thu, 10 Nov 2016 15:56:56 -0500 Subject: [PATCH] More intuitive configuration composition (#1622) --- src/configuration.ts | 51 ++++++++++--------- src/utils.ts | 6 +++ .../tslint-custom-rules-with-two-dirs.json | 6 ++- test/configurationTests.ts | 39 ++++++++------ test/executable/executableTests.ts | 8 --- test/ruleLoaderTests.ts | 7 +-- 6 files changed, 64 insertions(+), 53 deletions(-) diff --git a/src/configuration.ts b/src/configuration.ts index 3a8454c6a01..4a7e178ba9f 100644 --- a/src/configuration.ts +++ b/src/configuration.ts @@ -170,14 +170,14 @@ export function loadConfigurationFromPath(configFilePath: string): IConfiguratio const configFileDir = path.dirname(resolvedConfigFilePath); configFile.rulesDirectory = getRulesDirectories(configFile.rulesDirectory, configFileDir); - configFile.extends = arrayify(configFile.extends); + // load configurations, in order, using their identifiers or relative paths + // apply the current configuration last by placing it last in this array + const configs = arrayify(configFile.extends).map((name) => { + const nextConfigFilePath = resolveConfigurationPath(name, configFileDir); + return loadConfigurationFromPath(nextConfigFilePath); + }).concat([configFile]); - for (const name of configFile.extends) { - const baseConfigFilePath = resolveConfigurationPath(name, configFileDir); - const baseConfigFile = loadConfigurationFromPath(baseConfigFilePath); - configFile = extendConfigurationFile(configFile, baseConfigFile); - } - return configFile; + return configs.reduce(extendConfigurationFile, {}); } } @@ -211,28 +211,29 @@ function resolveConfigurationPath(filePath: string, relativeTo?: string) { } } -export function extendConfigurationFile(config: IConfigurationFile, baseConfig: IConfigurationFile): IConfigurationFile { +export function extendConfigurationFile(targetConfig: IConfigurationFile, + nextConfigSource: IConfigurationFile): IConfigurationFile { let combinedConfig: IConfigurationFile = {}; - const baseRulesDirectory = arrayify(baseConfig.rulesDirectory); - const configRulesDirectory = arrayify(config.rulesDirectory); - combinedConfig.rulesDirectory = configRulesDirectory.concat(baseRulesDirectory); + const configRulesDirectory = arrayify(targetConfig.rulesDirectory); + const nextConfigRulesDirectory = arrayify(nextConfigSource.rulesDirectory); + combinedConfig.rulesDirectory = configRulesDirectory.concat(nextConfigRulesDirectory); - combinedConfig.rules = {}; - for (const name of Object.keys(objectify(baseConfig.rules))) { - combinedConfig.rules[name] = baseConfig.rules[name]; - } - for (const name of Object.keys(objectify(config.rules))) { - combinedConfig.rules[name] = config.rules[name]; - } + const combineProperties = (targetProperty: any, nextProperty: any) => { + let combinedProperty: any = {}; + for (const name of Object.keys(objectify(targetProperty))) { + combinedProperty[name] = targetProperty[name]; + } + // next config source overwrites the target config object + for (const name of Object.keys(objectify(nextProperty))) { + combinedProperty[name] = nextProperty[name]; + } + return combinedProperty; + }; - combinedConfig.jsRules = {}; - for (const name of Object.keys(objectify(baseConfig.jsRules))) { - combinedConfig.jsRules[name] = baseConfig.jsRules[name]; - } - for (const name of Object.keys(objectify(config.jsRules))) { - combinedConfig.jsRules[name] = config.jsRules[name]; - } + combinedConfig.rules = combineProperties(targetConfig.rules, nextConfigSource.rules); + combinedConfig.jsRules = combineProperties(targetConfig.jsRules, nextConfigSource.jsRules); + combinedConfig.linterOptions = combineProperties(targetConfig.linterOptions, nextConfigSource.linterOptions); return combinedConfig; } diff --git a/src/utils.ts b/src/utils.ts index 20743afd41a..c8b58e36c79 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -15,6 +15,9 @@ * limitations under the License. */ +/** + * Enforces the invariant that the input is an array. + */ export function arrayify(arg: T | T[]): T[] { if (Array.isArray(arg)) { return arg; @@ -25,6 +28,9 @@ export function arrayify(arg: T | T[]): T[] { } } +/** + * Enforces the invariant that the input is an object. + */ export function objectify(arg: any): any { if (typeof arg === "object" && arg != null) { return arg; diff --git a/test/config/tslint-custom-rules-with-two-dirs.json b/test/config/tslint-custom-rules-with-two-dirs.json index 6cc0107305f..a43cf3db773 100644 --- a/test/config/tslint-custom-rules-with-two-dirs.json +++ b/test/config/tslint-custom-rules-with-two-dirs.json @@ -2,10 +2,12 @@ "rulesDirectory": ["../files/custom-rules-2", "../files/custom-rules/"], "jsRules": { "always-fail": true, - "no-fail": true + "no-fail": true, + "rule-two": true }, "rules": { "always-fail": true, - "no-fail": true + "no-fail": true, + "rule-two": true } } diff --git a/test/configurationTests.ts b/test/configurationTests.ts index c73f0b7cdb8..1a4d28eda2a 100644 --- a/test/configurationTests.ts +++ b/test/configurationTests.ts @@ -23,6 +23,7 @@ describe("Configuration", () => { it("extendConfigurationFile", () => { const EMPTY_CONFIG: IConfigurationFile = { jsRules: {}, + linterOptions: {}, rules: {}, rulesDirectory: [], }; @@ -32,72 +33,80 @@ describe("Configuration", () => { assert.deepEqual(extendConfigurationFile(EMPTY_CONFIG, {}), EMPTY_CONFIG); assert.deepEqual(extendConfigurationFile({}, { jsRules: { row: "oar" }, + linterOptions: {}, rules: { foo: "bar" }, rulesDirectory: "foo", }), { jsRules: { row: "oar" }, + linterOptions: {}, rules: {foo: "bar"}, rulesDirectory: ["foo"], }); - assert.deepEqual(extendConfigurationFile({ + const actualConfig = extendConfigurationFile({ jsRules: { row: "oar" }, + linterOptions: {}, rules: { a: 1, - b: 2, + b: 1, }, rulesDirectory: ["foo", "bar"], }, { jsRules: { fly: "wings" }, + linterOptions: {}, rules: { - b: 1, + b: 2, c: 3, }, rulesDirectory: "baz", - }), { + }); + /* tslint:disable:object-literal-sort-keys */ + const expectedConfig = { jsRules: { - fly: "wings", row: "oar", + fly: "wings", }, + linterOptions: {}, rules: { a: 1, b: 2, c: 3, }, rulesDirectory: ["foo", "bar", "baz"], - }); + }; + assert.deepEqual(actualConfig, expectedConfig); }); describe("loadConfigurationFromPath", () => { it("extends with relative path", () => { - let config = loadConfigurationFromPath("./test/config/tslint-extends-relative.json"); + const config = loadConfigurationFromPath("./test/config/tslint-extends-relative.json"); assert.isArray(config.rulesDirectory); - assert.isTrue(config.rules["no-fail"]); - assert.isFalse(config.rules["always-fail"]); + assert.isTrue(config.rules["no-fail"], "did not pick up 'no-fail' in base config"); + assert.isFalse(config.rules["always-fail"], "did not set 'always-fail' in top config"); assert.isTrue(config.jsRules["no-fail"]); assert.isFalse(config.jsRules["always-fail"]); }); it("extends with package", () => { - let config = loadConfigurationFromPath("./test/config/tslint-extends-package.json"); + const config = loadConfigurationFromPath("./test/config/tslint-extends-package.json"); assert.isArray(config.rulesDirectory); /* tslint:disable:object-literal-sort-keys */ assert.deepEqual(config.jsRules, { "rule-one": true, - "rule-two": true, "rule-three": false, + "rule-two": true, }); assert.deepEqual(config.rules, { "rule-one": true, - "rule-two": true, "rule-three": false, + "rule-two": true, }); /* tslint:enable:object-literal-sort-keys */ }); it("extends with package without customization", () => { - let config = loadConfigurationFromPath("./test/config/tslint-extends-package-no-mod.json"); + const config = loadConfigurationFromPath("./test/config/tslint-extends-package-no-mod.json"); assert.isArray(config.rulesDirectory); assert.deepEqual(config.jsRules, { @@ -171,13 +180,13 @@ describe("Configuration", () => { "always-fail": false, "no-fail": true, "rule-one": true, - "rule-two": false, + "rule-two": true, }); assert.deepEqual(config.rules, { "always-fail": false, "no-fail": true, "rule-one": true, - "rule-two": false, + "rule-two": true, }); }); diff --git a/test/executable/executableTests.ts b/test/executable/executableTests.ts index 50431bde133..258ae2ba50a 100644 --- a/test/executable/executableTests.ts +++ b/test/executable/executableTests.ts @@ -106,14 +106,6 @@ describe("Executable", function() { done(); }); }); - - it("exits with code 2 if several custom rules directories are specified in config file and file contains lint errors", (done) => { - execCli(["-c", "./test/config/tslint-custom-rules-with-two-dirs.json", "src/tslint.ts"], (err) => { - assert.isNotNull(err, "process should exit with error"); - assert.strictEqual(err.code, 2, "error code should be 2"); - done(); - }); - }); }); describe("--fix flag", () => { diff --git a/test/ruleLoaderTests.ts b/test/ruleLoaderTests.ts index 70324afd3bb..daa8b1f6422 100644 --- a/test/ruleLoaderTests.ts +++ b/test/ruleLoaderTests.ts @@ -1,4 +1,5 @@ -/* +/** + * @license * Copyright 2013 Palantir Technologies, Inc. * * Licensed under the Apache License, Version 2.0 (the "License"); @@ -40,7 +41,7 @@ describe("Rule Loader", () => { assert.throws( () => loadRules(invalidConfiguration, {}, RULES_DIRECTORY), - /invalidConfig1\ninvalidConfig2/ + /invalidConfig1\ninvalidConfig2/, ); }); @@ -56,7 +57,7 @@ describe("Rule Loader", () => { assert.throws( () => loadRules(invalidConfiguration, {}, RULES_DIRECTORY), - /_indent\nforin_\n-quotemark\neofline-/ + /_indent\nforin_\n-quotemark\neofline-/, ); });