Repository: lesshint/lesshint
Pull request #444 (2017-12-06 18:27:12)

Enhanced inline comments
<!--
If you're submitting a PR for a new feature, please open an issue about it first so we can discuss it.
If you're submitting a PR for something else, for example a documentation fix, go right ahead!
-->

**Which issue, if any, does this resolve?**
#404

**Is there anything in this PR that needs extra explaining or should something specific be focused on?**
It's a rather large change, so an extra pair of eyes would be appreciated!


Patch:
diff --git a/docs/user-guide/configuration.md b/docs/user-guide/configuration.md
index 5be7f8c..e1ac8eb 100644
--- a/docs/user-guide/configuration.md
+++ b/docs/user-guide/configuration.md
@@ -23,7 +23,66 @@ In the `.lesshintrc` file, each option is specified by its own JSON object, for
 Now, take a look at the available [linter options](/lib/linters/README.md).
 
 ## Inline configuration
-It's possible to configure rules using inline comments in your `.less` files. This needs to be on the first line of your `.less` file. For example:
+Sometimes, you may want to temporarily disable one or more rules in your `.less` files. This can be accomplished with the use of special inline configuration comments. For example:
+
+```less
+.foo {
+    color: red !important; // Will be reported since it's outside a disable/enable range
+}
+
+// lesshint-disable
+.bar {
+    color: blue !important; // This won't be reported
+}
+// lesshint-enable
+
+.baz {
+    color: green !important; // This will also be reported since it's outside the disable/enable range
+}
+```
+
+It's also possible to disable and enable a specific rule:
+
+```less
+// lesshint-disable importantRule
+.bar {
+    color: blue !important; // This won't be reported
+}
+// lesshint-enable importantRule
+```
+
+Multiple rules can be disabled and enabled at once. It's even possible to disable multiple rules and then just enabling one of them. For example:
+
+```less
+// lesshint-disable emptyRule, spaceBeforeBrace
+.foo{
+    // Won't report anything here
+}
+// lesshint-enable emptyRule
+.bar{
+    // Will report this empty rule, but not the missing space before the brace
+}
+```
+
+To disable rules on the current or next line, use `lesshint-disable-line` and `lesshint-disable-next-line`. They can of course also accept a single rule to disable.
+
+```less
+.foo {
+    color: red !important; // lesshint-disable-line
+}
+
+// lesshint-disable-next-line universalSelector
+* {
+    box-sizing: border-box;
+}
+```
+
+__Note:__ Comments in selector and value lists are currently ignored.
+
+### Old inline configuration style
+While we still support the old style of inline configuration comments, they are deprecated and their usage highly discouraged in favor of the new style documented above.
+
+When using the old style of inline configuration comments, they need to be on the first line of your `.less` file. For example:
 
 ```less
 // lesshint spaceBeforeBrace: false
diff --git a/lib/config/defaults.json b/lib/config/defaults.json
index e922baa..4496f3e 100644
--- a/lib/config/defaults.json
+++ b/lib/config/defaults.json
@@ -205,6 +205,7 @@
     "variableValue": {
         "enabled": true,
         "always": [],
-        "never": []
+        "never": [],
+        "allowedValues": []
     }
 }
diff --git a/lib/linter.js b/lib/linter.js
index f95a5fa..48ba88c 100644
--- a/lib/linter.js
+++ b/lib/linter.js
@@ -76,6 +76,7 @@ class Linter {
         };
 
         this.configure();
+        this.fetchInlineRules();
     }
 
     checkNode (node) {
@@ -102,6 +103,8 @@ class Linter {
                 lint.forEach((piece) => {
                     const column = piece.column || node.source.start.column;
                     const line = piece.line || node.source.start.line;
+
+                    // Old, deprecated way of single line inline configuration comments
                     const shouldIgnore = this.lineOptions.some((lineOption) => {
                         lineOption = lineOption[linter.name];
 
@@ -141,9 +144,9 @@ class Linter {
 
     configure () {
         this.linters = this.setupLinters(defaultLinters);
-        this.lineOptions = [];
 
-        // Fetch all single line options
+        // Fetch all single line options, deprecated
+        this.lineOptions = [];
         this.ast.root.walkComments((node) => {
             const option = this.parseOption(node);
             const key = Object.keys(option)[0];
@@ -161,6 +164,110 @@ class Linter {
         this.options = merge({}, this.options, inlineOptions);
     }
 
+    excludeResults () {
+        this.results = this.results.filter((result) => {
+            const linter = result.linter;
+            const rules = this.inlineRules[linter] || this.inlineRules.all;
+
+            if (!rules) {
+                return true;
+            }
+
+            for (const rule of rules) {
+                return !(result.line >= rule.start && (result.line <= rule.end || rule.end === undefined));
+            }
+
+            return true;
+        });
+    }
+
+    fetchInlineRules () {
+        this.inlineRules = {};
+
+        this.ast.root.walkComments((comment) => {
+            const text = comment.text;
+
+            if (!(/^\s*lesshint-.+/.test(text))) {
+                return;
+            }
+
+            const line = comment.source.start.line;
+            const match = text.match(/^\s*lesshint-[^\s]+/)[0];
+            const cmd = match.replace('lesshint-', '');
+
+            let rules;
+
+            rules = text.slice(match.length).split(',');
+            rules = rules.map((rule) => {
+                return rule.trim();
+            });
+            rules = rules.filter((rule) => {
+                return !!rule;
+            });
+
+            if (!rules.length) {
+                rules = ['all'];
+            }
+
+            switch (cmd) {
+                case 'disable':
+                    rules.forEach((rule) => {
+                        const options = {
+                            start: line
+                        };
+
+                        this.inlineRules[rule] = this.inlineRules[rule] || [];
+                        this.inlineRules[rule] = this.inlineRules[rule].concat(options);
+                    });
+
+                    break;
+                case 'enable':
+                    rules.forEach((rule) => {
+                        const length = this.inlineRules[rule].length;
+
+                        this.inlineRules[rule][length - 1].end = line;
+                    });
+
+                    break;
+                case 'disable-line':
+                    rules.forEach((rule) => {
+                        const options = {
+                            end: line,
+                            start: line
+                        };
+
+                        this.inlineRules[rule] = this.inlineRules[rule] || [];
+                        this.inlineRules[rule] = this.inlineRules[rule].concat(options);
+                    });
+
+                    break;
+                case 'disable-next-line':
+                    rules.forEach((rule) => {
+                        const options = {
+                            end: line + 1,
+                            start: line + 1
+                        };
+
+                        this.inlineRules[rule] = this.inlineRules[rule] || [];
+                        this.inlineRules[rule] = this.inlineRules[rule].concat(options);
+                    });
+
+                    break;
+                default:
+                    this.results.push({
+                        column: comment.source.start.column,
+                        file: this.filename,
+                        fullPath: this.fullPath,
+                        line: line,
+                        linter: 'parse error',
+                        message: `Invalid inline configuration comment: "${ text }"`,
+                        severity: 'error'
+                    });
+            }
+        });
+    }
+
+    // Old, deprecated way of handling inline configuration comments
     getInlineOptions (node) {
         node = node || {};
 
@@ -190,6 +297,8 @@ class Linter {
             throw e;
         }
 
+        this.excludeResults();
+
         this.results = sortBy(this.results, 'column');
         this.results = sortBy(this.results, 'line');
 
diff --git a/lib/linters/README.md b/lib/linters/README.md
index 7b80167..0faa77b 100644
--- a/lib/linters/README.md
+++ b/lib/linters/README.md
@@ -975,9 +975,10 @@ The [CSS spec](http://dev.w3.org/csswg/css-values/#url-value) also recommends th
 Force variable or non variable value in property.
 
 Option     | Description
----------- | ----------
-`always`    | Array of property names
-`never`    | Array of property names
+--------------- | ----------
+`always`        | Array of property names
+`never`         | Array of property names
+`allowedValues` | Array of allowed non variables values
 
 ### always ['color'] valid
 ```less
@@ -986,6 +987,13 @@ Option     | Description
 }
 ```
 
+### always ['color'] allowedValues ['none'] valid
+```less
+.foo {
+    color: none;
+}
+```
+
 ### always ['color'] invalid
 ```less
 .foo {
diff --git a/lib/linters/variable_value.js b/lib/linters/variable_value.js
index 467b3c6..57efc15 100644
--- a/lib/linters/variable_value.js
+++ b/lib/linters/variable_value.js
@@ -9,6 +9,11 @@ module.exports = {
     message: '%s is not allowed in "%s" property.',
 
     lint: function variableValueLinter (config, node) {
+        if (config.allowedValues &&
+            config.allowedValues.indexOf(node.value) !== -1) {
+            return;
+        }
+
         if (config.never && config.never.indexOf(node.prop) !== -1) {
             if (isVariable(node.value)) {
                 const position = node.positionBy({


Test patch:
diff --git a/test/data/inline-options/endlines.less b/test/data/endlines.less
similarity index 100%
rename from test/data/inline-options/endlines.less
rename to test/data/endlines.less
diff --git a/test/data/inline-options/file-options-all.less b/test/data/inline-options/file-options-all.less
new file mode 100644
index 0000000..e2c5c2f
--- /dev/null
+++ b/test/data/inline-options/file-options-all.less
@@ -0,0 +1,13 @@
+// lesshint-disable
+.foo {
+    color:red;
+}
+
+.bar {
+
+}
+// lesshint-enable
+
+* {
+    box-sizing: border-box;
+}
diff --git a/test/data/inline-options/file-options.less b/test/data/inline-options/file-options.less
new file mode 100644
index 0000000..b644a76
--- /dev/null
+++ b/test/data/inline-options/file-options.less
@@ -0,0 +1,13 @@
+// lesshint-disable spaceAfterPropertyColon, emptyRule
+.foo {
+    color:red;
+}
+
+.bar{
+
+}
+// lesshint-enable emptyRule
+
+.baz {
+
+}
diff --git a/test/data/inline-options/line-options.less b/test/data/inline-options/line-options.less
index 87a4804..ec4e08c 100644
--- a/test/data/inline-options/line-options.less
+++ b/test/data/inline-options/line-options.less
@@ -1,11 +1,13 @@
-.foo{ // lesshint spaceBeforeBrace: false
+// lesshint-disable-next-line spaceBeforeBrace
+.foo{
     color: red;
 }
 
 .bar{
-    color:blue; // lesshint spaceAfterPropertyColon: false
+    color:blue; // lesshint-disable-line spaceAfterPropertyColon
+    margin-left:10px;
 }
 
 .baz {
-    color: #FFF; // lesshint hexLength: false, hexNotation: false
+    color: #FFF; // lesshint-disable-line hexLength, hexNotation
 }
diff --git a/test/data/inline-options/options-invalid.less b/test/data/inline-options/options-invalid.less
index 242b135..b746741 100755
--- a/test/data/inline-options/options-invalid.less
+++ b/test/data/inline-options/options-invalid.less
@@ -1,1 +1,1 @@
-// lesshint fnord
+// lesshint-disbale foobar
diff --git a/test/data/inline-options/file-options-disabled.less b/test/data/old-inline-options/file-options-disabled.less
similarity index 100%
rename from test/data/inline-options/file-options-disabled.less
rename to test/data/old-inline-options/file-options-disabled.less
diff --git a/test/data/inline-options/file-options-enabled.less b/test/data/old-inline-options/file-options-enabled.less
similarity index 100%
rename from test/data/inline-options/file-options-enabled.less
rename to test/data/old-inline-options/file-options-enabled.less
diff --git a/test/data/old-inline-options/line-options.less b/test/data/old-inline-options/line-options.less
new file mode 100644
index 0000000..87a4804
--- /dev/null
+++ b/test/data/old-inline-options/line-options.less
@@ -0,0 +1,11 @@
+.foo{ // lesshint spaceBeforeBrace: false
+    color: red;
+}
+
+.bar{
+    color:blue; // lesshint spaceAfterPropertyColon: false
+}
+
+.baz {
+    color: #FFF; // lesshint hexLength: false, hexNotation: false
+}
diff --git a/test/data/old-inline-options/options-invalid.less b/test/data/old-inline-options/options-invalid.less
new file mode 100755
index 0000000..242b135
--- /dev/null
+++ b/test/data/old-inline-options/options-invalid.less
@@ -0,0 +1,1 @@
+// lesshint fnord
diff --git a/test/specs/linter.js b/test/specs/linter.js
index e7e6c25..62f82d9 100644
--- a/test/specs/linter.js
+++ b/test/specs/linter.js
@@ -232,9 +232,143 @@ describe('linter', function () {
             expect(result).to.deep.equal(expected);
         });
 
-        it('should only care about the first inline comment', function () {
-            const testPath = path.resolve(process.cwd(), './test/data/inline-options/file-options-enabled.less');
-            const source = readFileSync('./test/data/inline-options/file-options-enabled.less');
+        it('should honor inline configuration comments', function () {
+            const testPath = path.resolve(process.cwd(), './test/data/inline-options/file-options.less');
+            const source = readFileSync('./test/data/inline-options/file-options.less');
+            const expected = [{
+                column: 1,
+                file: 'file-options.less',
+                fullPath: testPath,
+                line: 11,
+                linter: 'emptyRule',
+                message: "There shouldn't be any empty rules present.",
+                position: 119,
+                severity: 'warning',
+                source: '.baz {'
+            }];
+
+            const config = {
+                emptyRule: true,
+                spaceAfterPropertyColon: {
+                    enabled: true,
+                    style: 'one_space'
+                }
+            };
+
+            const linter = new Linter(source, testPath, config);
+            const result = linter.lint();
+
+            expect(result).to.deep.equal(expected);
+        });
+
+        it('should allow inline disabling/enabling of all rules', function () {
+            const testPath = path.resolve(process.cwd(), './test/data/inline-options/file-options-all.less');
+            const source = readFileSync('./test/data/inline-options/file-options-all.less');
+            const expected = [{
+                column: 1,
+                file: 'file-options-all.less',
+                fullPath: testPath,
+                line: 11,
+                linter: 'universalSelector',
+                message: "The universal selector shouldn't be used.",
+                position: 75,
+                severity: 'warning',
+                source: '* {'
+            }];
+
+            const config = {
+                emptyRule: true,
+                spaceAfterPropertyColon: {
+                    enabled: true,
+                    style: 'one_space'
+                },
+                universalSelector: true
+            };
+
+            const linter = new Linter(source, testPath, config);
+            const result = linter.lint();
+
+            expect(result).to.deep.equal(expected);
+        });
+
+        it('should honor single line inline configuration comments', function () {
+            const testPath = path.resolve(process.cwd(), './test/data/inline-options/line-options.less');
+            const source = readFileSync('./test/data/inline-options/line-options.less');
+            const expected = [
+                {
+                    column: 5,
+                    file: 'line-options.less',
+                    fullPath: testPath,
+                    line: 6,
+                    linter: 'spaceBeforeBrace',
+                    message: 'Opening curly brace should be preceded by one space.',
+                    position: 76,
+                    severity: 'warning',
+                    source: '.bar{'
+                },
+                {
+                    column: 17,
+                    file: 'line-options.less',
+                    fullPath: testPath,
+                    line: 8,
+                    linter: 'spaceAfterPropertyColon',
+                    message: 'Colon after property name should be followed by one space.',
+                    position: 159,
+                    severity: 'warning',
+                    source: '    margin-left:10px;'
+                }
+            ];
+
+            const config = {
+                hexLength: {
+                    enabled: true,
+                    style: 'long'
+                },
+                hexNotation: {
+                    enabled: true,
+                    style: 'lowercase'
+                },
+                spaceAfterPropertyColon: {
+                    enabled: true,
+                    style: 'one_space'
+                },
+                spaceBeforeBrace: {
+                    enabled: true,
+                    style: 'one_space'
+                },
+                universalSelector: true
+            };
+
+            const linter = new Linter(source, testPath, config);
+            const result = linter.lint();
+
+            expect(result).to.deep.equal(expected);
+        });
+
+        it('should report invalid inline configuration comments', function () {
+            const testPath = path.resolve(process.cwd(), './test/data/inline-options/options-invalid.less');
+            const source = readFileSync('./test/data/inline-options/options-invalid.less');
+            const expected = [{
+                column: 1,
+                file: 'options-invalid.less',
+                fullPath: testPath,
+                line: 1,
+                linter: 'parse error',
+                message: 'Invalid inline configuration comment: "lesshint-disbale foobar"',
+                severity: 'error'
+            }];
+
+            const config = {};
+
+            const linter = new Linter(source, testPath, config);
+            const result = linter.lint();
+
+            expect(result).to.deep.equal(expected);
+        });
+
+        it('should only care about the first inline comment (old style)', function () {
+            const testPath = path.resolve(process.cwd(), './test/data/old-inline-options/file-options-enabled.less');
+            const source = readFileSync('./test/data/old-inline-options/file-options-enabled.less');
             const expected = [{
                 column: 5,
                 file: 'file-options-enabled.less',
@@ -265,9 +399,9 @@ describe('linter', function () {
             expect(result).to.deep.equal(expected);
         });
 
-        it('should not report rules that are disabled inline', function () {
-            const testPath = path.resolve(__dirname, './test/data/inline-options/file-options-enabled.less');
-            const source = readFileSync('./test/data/inline-options/file-options-enabled.less');
+        it('should not report rules that are disabled inline (old style)', function () {
+            const testPath = path.resolve(__dirname, './test/data/old-inline-options/file-options-enabled.less');
+            const source = readFileSync('./test/data/old-inline-options/file-options-enabled.less');
             const expected = [{
                 column: 5,
                 file: 'file-options-enabled.less',
@@ -298,9 +432,9 @@ describe('linter', function () {
             expect(result).to.deep.equal(expected);
         });
 
-        it('should allow disabled rules to be enabled inline', function () {
-            const testPath = path.resolve(__dirname, './test/data/inline-options/file-options-disabled.less');
-            const source = readFileSync('./test/data/inline-options/file-options-disabled.less');
+        it('should allow disabled rules to be enabled inline (old style)', function () {
+            const testPath = path.resolve(__dirname, './test/data/old-inline-options/file-options-disabled.less');
+            const source = readFileSync('./test/data/old-inline-options/file-options-disabled.less');
             const expected = [{
                 column: 11,
                 file: 'file-options-disabled.less',
@@ -338,9 +472,9 @@ describe('linter', function () {
             expect(result).to.deep.equal(expected);
         });
 
-        it('should not report rules that are disabled on a single line', function () {
-            const testPath = path.resolve(__dirname, './test/data/inline-options/line-options.less');
-            const source = readFileSync('./test/data/inline-options/line-options.less');
+        it('should not report rules that are disabled on a single line (old style)', function () {
+            const testPath = path.resolve(__dirname, './test/data/old-inline-options/line-options.less');
+            const source = readFileSync('./test/data/old-inline-options/line-options.less');
             const expected = [{
                 column: 5,
                 file: 'line-options.less',
@@ -366,9 +500,19 @@ describe('linter', function () {
             expect(result).to.deep.equal(expected);
         });
 
+        it('should report invalid inline options (old style)', function () {
+            const source = readFileSync('./test/data/old-inline-options/options-invalid.less');
+
+            expect(function () {
+                const linter = new Linter(source, 'options-invalid.less', {});
+
+                linter.lint();
+            }).to.throw('Invalid inline option on line 1');
+        });
+
         it('reports positions after multiple consecutive endline characters', function () {
-            const testPath = path.resolve(__dirname, './test/data/inline-options/endlines.less');
-            const source = readFileSync('./test/data/inline-options/endlines.less');
+            const testPath = path.resolve(__dirname, './test/data/endlines.less');
+            const source = readFileSync('./test/data/endlines.less');
             const expected = [{
                 column: 5,
                 file: 'endlines.less',
@@ -394,16 +538,6 @@ describe('linter', function () {
             expect(result).to.deep.equal(expected);
         });
 
-        it('should report invalid inline options', function () {
-            const source = readFileSync('./test/data/inline-options/options-invalid.less');
-
-            expect(function () {
-                const linter = new Linter(source, 'options-invalid.less', {});
-
-                linter.lint();
-            }).to.throw('Invalid inline option on line 1');
-        });
-
         it('should not report comma spaces for selectors that have pseudos', function () {
             const source = '.foo,\n.bar:not(.foo){}';
             const testPath = 'test.less';
diff --git a/test/specs/linters/variable_value.js b/test/specs/linters/variable_value.js
index 22a037b..c804d5e 100644
--- a/test/specs/linters/variable_value.js
+++ b/test/specs/linters/variable_value.js
@@ -100,5 +100,19 @@ describe('lesshint', function () {
                 expect(result).to.be.undefined;
             });
         });
+
+        it('should allow value if in allowedValues list', function () {
+            const source = 'color: none;';
+            const options = {
+                always: ['color'],
+                allowedValues: ['none']
+            };
+
+            return spec.parse(source, function (ast) {
+                const result = spec.linter.lint(options, ast.root.first);
+
+                expect(result).to.be.undefined;
+            });
+        });
     });
 });