More intuitive configuration composition (#1622)

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;
 }

src/utils.ts

@@ -15,6 +15,9 @@
  * limitations under the License.
  */
 
+/**
+ * Enforces the invariant that the input is an array.
+ */
 export function arrayify<T>(arg: T | T[]): T[] {
     if (Array.isArray(arg)) {
         return arg;
@@ -25,6 +28,9 @@ export function arrayify<T>(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;

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
   }
 }

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,
             });
         });
 

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", () => {

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-/,
         );
     });