Repository: eslint/eslint
Pull request #9416 (2017-10-12 09:44:11)

Update: Don't ignore comments (no-trailing-spaces)
[X] Changes an existing rule ([template](https://raw.githubusercontent.com/eslint/eslint/master/templates/rule-change-proposal.md))

**What rule do you want to change?**
no-trailing-spaces

**Does this change cause the rule to produce more or fewer warnings?**
More

**How will the change be implemented? (New option, new default behavior, etc.)?**
New default behavior

**Please provide some example code that this change will affect:**

```js
// Comment with trailing space 
```

**What does the rule currently do for this code?**
By default trailing spaces in comments are ignored.

**What will the rule do after it's changed?**
As specified in the docs (https://eslint.org/docs/rules/no-trailing-spaces) the comments are also checked for trailing whitespace by default.

**What changes did you make? (Give an overview)**
- Update the code to invert the check for absence of the options.ignoreComments. The variable ignoreComments will now be **false** if the option is absent, instead of **true**.
- Add test for this rule, focusssing on default behavior.
- Fix 3 instances of this error in lib/rules/generator-star-spacing.js to fix test errors.

**Is there anything you'd like reviewers to focus on?**
If my assumption is correct that the documentation described the preferred behavior.

eslintbot: LGTM
kaicataldo: This is a breaking change and seems like a common enough error that it could break a lot of people's linting tasks. I know we reserve the right to add this in semver-minor releases, but I'd at least like to discuss as a team and do it intentionally :)

The alternative to this would be to update the documentation for now and defer this change until our next major release (I also think it should be the default behavior).
kaicataldo: **TSC Summary:**
This is a breaking change for a case that I feel like could be very common.

**Question:**
Do we want to include this in a semver-minor release? Or should we update the documentation to accurately reflect the current behavior and defer this change for our next major release?

not-an-aardvark: The documentation clearly states the correct behavior, and the current implementation does not follow the documented behavior. In my opinion, this is a clear case where it's just a bugfix in a rule that results in more errors, which would be a semver-minor change according to our semver policy.

Note that this issue was introduced in when the `ignoreComments` option came out in v4.1.0  (which was in June). So I think the ecosystem impact won't be that bad, since it would only potentially affect code which has been modified in the last few months.
not-an-aardvark: In [today's meeting](https://gitter.im/eslint/tsc-meetings/archives/2017/10/12), the TSC decided to accept this as a semver-minor bugfix.

Patch:
diff --git a/lib/rules/generator-star-spacing.js b/lib/rules/generator-star-spacing.js
index 5c6120224..a718b59af 100644
--- a/lib/rules/generator-star-spacing.js
+++ b/lib/rules/generator-star-spacing.js
@@ -68,7 +68,7 @@ module.exports = {
 
         /**
          * Returns resolved option definitions based on an option and defaults
-         * 
+         *
          * @param {any} option - The option object or string value
          * @param {Object} defaults - The defaults to use if options are not present
          * @returns {Object} the resolved object definition
@@ -121,7 +121,7 @@ module.exports = {
 
         /**
          * Checks the spacing between two tokens before or after the star token.
-         * 
+         *
          * @param {string} kind Either "named", "anonymous", or "method"
          * @param {string} side Either "before" or "after".
          * @param {Token} leftToken `function` keyword token if side is "before", or
@@ -161,7 +161,7 @@ module.exports = {
 
         /**
          * Enforces the spacing around the star if node is a generator function.
-         * 
+         *
          * @param {ASTNode} node A function expression or declaration node.
          * @returns {void}
          */
diff --git a/lib/rules/no-trailing-spaces.js b/lib/rules/no-trailing-spaces.js
index b5d2f8d1b..598bbea4f 100644
--- a/lib/rules/no-trailing-spaces.js
+++ b/lib/rules/no-trailing-spaces.js
@@ -49,7 +49,7 @@ module.exports = {
 
         const options = context.options[0] || {},
             skipBlankLines = options.skipBlankLines || false,
-            ignoreComments = typeof options.ignoreComments === "undefined" || options.ignoreComments;
+            ignoreComments = typeof options.ignoreComments === "boolean" && options.ignoreComments;
 
         /**
          * Report the error message


Test patch:
diff --git a/tests/bench/bench.js b/tests/bench/bench.js
index ccae42fc6..be574cf33 100644
--- a/tests/bench/bench.js
+++ b/tests/bench/bench.js
@@ -33,7 +33,6 @@ function benchmark(grep, times) {
 
 function run(content, times) {
     while(times--) {
-        linter.reset();
         linter.verify(content, config);
     }
 }
diff --git a/tests/bench/xform-rules.js b/tests/bench/xform-rules.js
index fa13aa7f7..9456f3258 100644
--- a/tests/bench/xform-rules.js
+++ b/tests/bench/xform-rules.js
@@ -3,7 +3,7 @@ module.exports = transform;
 var fs = require("fs"),
     path = require("path"),
     through = require("through");
-    
+
 var target = path.resolve(__dirname, "..", "..", "lib", "load-rules.js");
 
 
@@ -23,10 +23,14 @@ function inject() {
     var output = "module.exports = function() {\n";
     output += "    var rules = Object.create(null);\n";
 
-    fs.readdirSync(path.resolve(__dirname, "..", "..", "lib", "rules")).forEach(function(filename) {
-        var basename = path.basename(filename, ".js");
-        output += "    rules[\"" + basename + "\"] = require(\"./rules/" + basename + "\");\n";
-    });
+    fs.readdirSync(path.resolve(__dirname, "..", "..", "lib", "rules"))
+        .filter(function(filename) {
+            return filename.endsWith(".js");
+        })
+        .forEach(function(filename) {
+            var basename = path.basename(filename, ".js");
+            output += "    rules[\"" + basename + "\"] = require(\"./rules/" + basename + "\");\n";
+        });
 
     output += "\n    return rules;\n};";
 
diff --git a/tests/lib/rules/no-trailing-spaces.js b/tests/lib/rules/no-trailing-spaces.js
index d33af0c25..37c81f327 100644
--- a/tests/lib/rules/no-trailing-spaces.js
+++ b/tests/lib/rules/no-trailing-spaces.js
@@ -71,6 +71,14 @@ ruleTester.run("no-trailing-spaces", rule, {
             code: "// Trailing comment test. ",
             options: [{ ignoreComments: true }]
         },
+        {
+            code: "// Trailing comment test.",
+            options: [{ ignoreComments: false }]
+        },
+        {
+            code: "// Trailing comment test.",
+            options: []
+        },
         {
             code: "/* \nTrailing comments test. \n*/",
             options: [{ ignoreComments: true }]
@@ -484,6 +492,19 @@ ruleTester.run("no-trailing-spaces", rule, {
                     column: 20
                 }
             ]
+        },
+        {
+            code: "// Trailing comment default test. ",
+            output: "// Trailing comment default test.",
+            options: [],
+            errors: [
+                {
+                    message: "Trailing spaces not allowed.",
+                    type: "Program",
+                    line: 1,
+                    column: 34
+                }
+            ]
         }
     ]
 });