Repository: eslint/eslint
Pull request #9224 (2017-09-04 03:38:49)

Chore: remove ConfigOps.normalize in favor of ConfigOps.getRuleSeverity
<!--
    ESLint adheres to the [JS Foundation Code of Conduct](https://js.foundation/community/code-of-conduct).
-->

**What is the purpose of this pull request? (put an "X" next to item)**

[x] Other, please explain:

<!--
    If the item you've checked above has a template, please paste the template questions below and answer them. (If this pull request is addressing an issue, you can just paste a link to the issue here instead.)
-->

<!--
    Please ensure your pull request is ready:

    - Read the pull request guide (http://eslint.org/docs/developer-guide/contributing/pull-requests)
    - Include tests for this change
    - Update documentation for this change (if appropriate)
-->

<!--
    The following is required for all pull requests:
-->

**What changes did you make? (Give an overview)**

This removes the `normalize` method from `config-ops` in favor of a `getRuleSeverity` method.

Previously, `ConfigOps.normalize` would only convert string severities to numeric severities, but it would still leave severities in arrays, and `Linter` used a separate `getRuleSeverity` function to extract them. Now `ConfigOps.getRuleSeverity` handles both operations, so the `getRuleSeverity` function in `Linter` is redundant.

The goal of this change is to (a) reduce the number of places where modules mutate each others' objects, and (b) organize the rule severity logic into one place.

**Is there anything you'd like reviewers to focus on?**

Nothing in particular


eslintbot: LGTM

Patch:
diff --git a/lib/config/config-ops.js b/lib/config/config-ops.js
index d169e60dc..4ed5ec6b0 100644
--- a/lib/config/config-ops.js
+++ b/lib/config/config-ops.js
@@ -193,25 +193,25 @@ module.exports = {
     },
 
     /**
-     * Converts new-style severity settings (off, warn, error) into old-style
-     * severity settings (0, 1, 2) for all rules. Assumption is that severity
-     * values have already been validated as correct.
-     * @param {Object} config The config object to normalize.
-     * @returns {void}
+     * Normalizes the severity value of a rule's configuration to a number
+     * @param {(number|string|[number, ...*]|[string, ...*])} ruleConfig A rule's configuration value, generally
+     * received from the user. A valid config value is either 0, 1, 2, the string "off" (treated the same as 0),
+     * the string "warn" (treated the same as 1), the string "error" (treated the same as 2), or an array
+     * whose first element is one of the above values. Strings are matched case-insensitively.
+     * @returns {(0|1|2)} The numeric severity value if the config value was valid, otherwise 0.
      */
-    normalize(config) {
+    getRuleSeverity(ruleConfig) {
+        const severityValue = Array.isArray(ruleConfig) ? ruleConfig[0] : ruleConfig;
 
-        if (config.rules) {
-            Object.keys(config.rules).forEach(ruleId => {
-                const ruleConfig = config.rules[ruleId];
+        if (severityValue === 0 || severityValue === 1 || severityValue === 2) {
+            return severityValue;
+        }
 
-                if (typeof ruleConfig === "string") {
-                    config.rules[ruleId] = RULE_SEVERITY[ruleConfig.toLowerCase()] || 0;
-                } else if (Array.isArray(ruleConfig) && typeof ruleConfig[0] === "string") {
-                    ruleConfig[0] = RULE_SEVERITY[ruleConfig[0].toLowerCase()] || 0;
-                }
-            });
+        if (typeof severityValue === "string") {
+            return RULE_SEVERITY[severityValue.toLowerCase()] || 0;
         }
+
+        return 0;
     },
 
     /**
diff --git a/lib/linter.js b/lib/linter.js
index 7552612a0..24008ad55 100755
--- a/lib/linter.js
+++ b/lib/linter.js
@@ -604,16 +604,6 @@ function stripUnicodeBOM(text) {
     return text;
 }
 
-/**
- * Get the severity level of a rule (0 - none, 1 - warning, 2 - error)
- * Returns 0 if the rule config is not valid (an Array or a number)
- * @param {Array|number} ruleConfig rule configuration
- * @returns {number} 0, 1, or 2, indicating rule severity
- */
-function getRuleSeverity(ruleConfig) {
-    return Array.isArray(ruleConfig) ? ruleConfig[0] : ruleConfig;
-}
-
 /**
  * Get the options for a rule (not including severity), if any
  * @param {Array|number} ruleConfig rule configuration
@@ -886,9 +876,6 @@ class Linter {
             modifyConfigResult.problems.forEach(problem => problems.push(problem));
         }
 
-        // ensure that severities are normalized in the config
-        ConfigOps.normalize(config);
-
         const emitter = new EventEmitter().setMaxListeners(Infinity);
 
         /*
@@ -928,7 +915,12 @@ class Linter {
         );
 
         // enable appropriate rules
-        Object.keys(config.rules).filter(ruleId => getRuleSeverity(config.rules[ruleId]) > 0).forEach(ruleId => {
+        Object.keys(config.rules).forEach(ruleId => {
+            const severity = ConfigOps.getRuleSeverity(config.rules[ruleId]);
+
+            if (severity === 0) {
+                return;
+            }
             let ruleCreator = this.rules.get(ruleId);
 
             if (!ruleCreator) {
@@ -942,7 +934,6 @@ class Linter {
                 this.rules.define(ruleId, ruleCreator);
             }
 
-            const severity = getRuleSeverity(config.rules[ruleId]);
             const ruleContext = Object.freeze(
                 Object.assign(
                     Object.create(sharedTraversalContext),


Test patch:
diff --git a/tests/lib/config/config-ops.js b/tests/lib/config/config-ops.js
index ed95238d0..faaff37f1 100644
--- a/tests/lib/config/config-ops.js
+++ b/tests/lib/config/config-ops.js
@@ -10,6 +10,7 @@
 
 const assert = require("chai").assert,
     leche = require("leche"),
+    util = require("util"),
     environments = require("../../../conf/environments"),
     Environments = require("../../../lib/config/environments"),
     ConfigCache = require("../../../lib/config/config-cache"),
@@ -550,150 +551,36 @@ describe("ConfigOps", () => {
         });
     });
 
-    describe("normalize()", () => {
-        it("should convert error rule setting to 2 when rule has just a severity", () => {
-            const config = {
-                rules: {
-                    foo: "errOr",
-                    bar: "error"
-                }
-            };
-
-            ConfigOps.normalize(config);
-
-            assert.deepEqual(config, {
-                rules: {
-                    foo: 2,
-                    bar: 2
-                }
-            });
-        });
-
-        it("should convert error rule setting to 2 when rule has array with severity", () => {
-            const config = {
-                rules: {
-                    foo: ["Error", "something"],
-                    bar: "error"
-                }
-            };
-
-            ConfigOps.normalize(config);
-
-            assert.deepEqual(config, {
-                rules: {
-                    foo: [2, "something"],
-                    bar: 2
-                }
-            });
-        });
-
-        it("should convert warn rule setting to 1 when rule has just a severity", () => {
-            const config = {
-                rules: {
-                    foo: "waRn",
-                    bar: "warn"
-                }
-            };
-
-            ConfigOps.normalize(config);
-
-            assert.deepEqual(config, {
-                rules: {
-                    foo: 1,
-                    bar: 1
-                }
-            });
-        });
-
-        it("should convert warn rule setting to 1 when rule has array with severity", () => {
-            const config = {
-                rules: {
-                    foo: ["Warn", "something"],
-                    bar: "warn"
-                }
-            };
-
-            ConfigOps.normalize(config);
-
-            assert.deepEqual(config, {
-                rules: {
-                    foo: [1, "something"],
-                    bar: 1
-                }
-            });
-        });
-
-        it("should convert off rule setting to 0 when rule has just a severity", () => {
-            const config = {
-                rules: {
-                    foo: "ofF",
-                    bar: "off"
-                }
-            };
-
-            ConfigOps.normalize(config);
-
-            assert.deepEqual(config, {
-                rules: {
-                    foo: 0,
-                    bar: 0
-                }
-            });
-        });
-
-        it("should convert off rule setting to 0 when rule has array with severity", () => {
-            const config = {
-                rules: {
-                    foo: ["Off", "something"],
-                    bar: "off"
-                }
-            };
-
-            ConfigOps.normalize(config);
-
-            assert.deepEqual(config, {
-                rules: {
-                    foo: [0, "something"],
-                    bar: 0
-                }
-            });
-        });
-
-        it("should convert invalid rule setting to 0 when rule has just a severity", () => {
-            const config = {
-                rules: {
-                    foo: "invalid",
-                    bar: "invalid"
-                }
-            };
-
-            ConfigOps.normalize(config);
-
-            assert.deepEqual(config, {
-                rules: {
-                    foo: 0,
-                    bar: 0
-                }
-            });
-        });
-
-        it("should convert invalid rule setting to 0 when rule has array with severity", () => {
-            const config = {
-                rules: {
-                    foo: ["invalid", "something"],
-                    bar: "invalid"
-                }
-            };
-
-            ConfigOps.normalize(config);
-
-            assert.deepEqual(config, {
-                rules: {
-                    foo: [0, "something"],
-                    bar: 0
-                }
-            });
-        });
+    describe("getRuleSeverity()", () => {
+        const EXPECTED_RESULTS = new Map([
+            [0, 0],
+            [1, 1],
+            [2, 2],
+            [[0], 0],
+            [[1], 1],
+            [[2], 2],
+            ["off", 0],
+            ["warn", 1],
+            ["error", 2],
+            [["off"], 0],
+            [["warn"], 1],
+            [["error"], 2],
+            ["OFF", 0],
+            ["wArN", 1],
+            [["ErRoR"], 2],
+            ["invalid config", 0],
+            [["invalid config"], 0],
+            [3, 0],
+            [[3], 0],
+            [1.5, 0],
+            [[1.5], 0]
+        ]);
+
+        for (const key of EXPECTED_RESULTS.keys()) {
+            it(`returns ${util.inspect(EXPECTED_RESULTS.get(key))} for ${util.inspect(key)}`, () => {
+                assert.strictEqual(ConfigOps.getRuleSeverity(key), EXPECTED_RESULTS.get(key));
+            });
+        }
     });
 
     describe("normalizeToStrings()", () => {