Repository: lesshint/lesshint
Pull request #374 (2017-04-13 16:04:40)

Add color variables linter
<!--
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?**
More related to #372 

**Is there anything in this PR that needs extra explaining or should something specific be focused on?**

This is adding a linter that prohibits hex colors unless used for variable assignment.


shellscape: I think it's awesome that folks are creating their own linters. That said, I do still think this is too specialized to be in core. Rather, I think the code is sound and that this would make a great third-party linter module. 
WattyRev: What would be your suggestion on making it accessible as a third-party linter module? Is there an established method of doing that?
shellscape: This https://github.com/Joshuakgoldberg/lesshint-browser-support is a good example (even though it's got TypeScript decorations) of a third party linter by @JoshuaKGoldberg.

I wouldn't close this PR just yet. This is only my opinion and the others should definitely weigh in first (and they may disagree with me)
jwilsson: I like it and think it's a good fit for core since we already got `variableValue`.

What does the rest of @lesshint/team think?

_(I've only skimmed the code real quick so if we decide to go ahead there might be a few comments there, just a heads up)_
kokarn: I'm not following. Doesn't the varibleValue rule do the same thing as this?
WattyRev: The variableValue rule enforces the use of variables for certain properties, while this rule prohibits the use of hard-coded hex colors except when defining a variable. 

It is good for my team because we have a predefined color pallet, so we want to make sure people use the existing color variables rather than adding their own. 
kokarn: Ah, I think I see now. 

Instead of having variableValue configured for each property where you might add a colour you want to just make sure colours are always a var.

Just out of curiosity, how many different properties would it be for variableValue?

```less
background-color
border-color
color
fill
```
WattyRev: Yes, but there are also the multi-part properies that would use color, like
background
border
box-shadow
etc. 

I think the variableValue rule is an all or none type thing, so mixing variables with other text doesnt fly
kokarn: Hmm, I kinda like it and kinda don't. 
Maybe it's the limited scope that gets me.

Something that does this but also covers stuff like measurements (em, px, rem, %) and durations might be something that feels a bit more general.

But I'm very undecided I'm afraid. I'll let @jwilsson handle this.

PS: Sorry for the long back and forth :)


jwilsson: @kokarn Could we get a Yes or No from you? 

Right now, we got one against and one in favor of, I'd want a majority for one or the other :)
kokarn: I said I'd leave it up to you. If you say yes, then it's a yes from me :)
jwilsson: Alright then, thanks @kokarn!

@WattyRev Really sorry for the delay on this one! Too much other things going on right now, but if you're still interested in finishing this up, I'll leave some small comments on the code. If not, we can still merge it and finish it up afterwards.
WattyRev: @jwilsson, no worries about the delay. I've made the changes you requested.

Patch:
diff --git a/lib/config/defaults.json b/lib/config/defaults.json
index 6f285bd..ef5f31c 100644
--- a/lib/config/defaults.json
+++ b/lib/config/defaults.json
@@ -12,6 +12,10 @@
         "enabled": true,
         "style": "zero"
     },
+    
+    "colorVariables": {
+        "enabled": false   
+    },
 
     "comment": {
         "allowed": "^!",
diff --git a/lib/linter.js b/lib/linter.js
index 1cad349..fcade73 100644
--- a/lib/linter.js
+++ b/lib/linter.js
@@ -9,6 +9,7 @@ const path = require('path');
 const defaultLinters = [
     require('./linters/attribute_quotes'),
     require('./linters/border_zero'),
+    require('./linters/color_variables'),
     require('./linters/comment'),
     require('./linters/decimal_zero'),
     require('./linters/depth_level'),
diff --git a/lib/linters/README.md b/lib/linters/README.md
index 645eaff..a206633 100644
--- a/lib/linters/README.md
+++ b/lib/linters/README.md
@@ -2,6 +2,7 @@
 
 * [attributeQuotes](#attributequotes)
 * [borderZero](#borderzero)
+* [colorVariables](#colorvariables)
 * [comment](#comment)
 * [decimalZero](#decimalzero)
 * [depthLevel](#depthlevel)
@@ -90,6 +91,23 @@ Option     | Description
 }
 ```
 
+## colorVariables
+Disallow hexidecimal colors when not used as a variable assignment.
+
+### invalid
+```less
+border: 1px solid #000;
+```
+
+### valid
+```less
+@black: #000;
+
+...
+
+border: 1px solid @black;
+```
+
 ## Comment
 Prefer single-line comments (`//`) over multi-line (`/* ... */`) since they're not rendered in the final CSS.
 This linter is disabled by default.
diff --git a/lib/linters/color_variables.js b/lib/linters/color_variables.js
new file mode 100644
index 0000000..1b64d07
--- /dev/null
+++ b/lib/linters/color_variables.js
@@ -0,0 +1,48 @@
+'use strict';
+
+const util = require('util');
+const parseValue = require('../utils/parse-value');
+const isVariable = require('../utils/is-variable');
+
+module.exports = {
+    name: 'colorVariables',
+    nodeTypes: ['decl'],
+    message: '%s should be replaced with an existing variable.',
+
+    lint: function colorVariablesLinter (config, node) {
+        /**
+         * Just in case, since postcss will sometimes include the semicolon
+         * in declaration values
+         */
+        node.value = node.value.replace(/;$/, '');
+
+        const ast = parseValue(node.value, {
+            loose: true
+        });
+
+        const results = [];
+        ast.first.walk((child) => {
+            // Only evaluate hex colors
+            if (child.type !== 'word' || !child.isColor) {
+                return;
+            }
+
+            // If this is a variable assignment, allow hex colors
+            const prop = node.prop;
+            if (isVariable(prop)) {
+                return;
+            }
+            const color = child.value;
+
+            // Hex color found, add to results
+            results.push({
+                column: node.source.start.column + node.prop.length + node.raws.between.length + (child.source.start.column - 1),
+                message: util.format(this.message, color)
+            });
+        });
+
+        if (results.length) {
+            return results;
+        }
+    }
+};


Test patch:
diff --git a/test/specs/linters/color_variables.js b/test/specs/linters/color_variables.js
new file mode 100644
index 0000000..9614a3e
--- /dev/null
+++ b/test/specs/linters/color_variables.js
@@ -0,0 +1,55 @@
+'use strict';
+
+const expect = require('chai').expect;
+const spec = require('../util.js').setup();
+
+describe('lesshint', function () {
+    describe('#colorVariables()', function () {
+        it('should have the proper node types', function () {
+            const source = 'color: #ABC;';
+
+            return spec.parse(source, function (ast) {
+                expect(spec.linter.nodeTypes).to.include(ast.root.first.type);
+            });
+        });
+
+        it('should not allow hex values', function () {
+            const source = 'color: #ABC;';
+            const expected = [{
+                column: 8,
+                message: '#ABC should be replaced with an existing variable.'
+            }];
+
+            return spec.parse(source, function (ast) {
+                const result = spec.linter.lint({}, ast.root.first);
+
+                expect(result).to.deep.equal(expected);
+            });
+        });
+
+        it('should allow hex values when assigned to a variable', function () {
+            const source = '@myvariable: #ABC;';
+            const expected = undefined;
+
+            return spec.parse(source, function (ast) {
+                const result = spec.linter.lint({}, ast.root.first);
+
+                expect(result).to.deep.equal(expected);
+            });
+        });
+
+        it('should should find hex values in multi part declarations', function () {
+            const source = 'border: 1px solid #ABC;';
+            const expected = [{
+                column: 19,
+                message: '#ABC should be replaced with an existing variable.'
+            }];
+
+            return spec.parse(source, function (ast) {
+                const result = spec.linter.lint({}, ast.root.first);
+
+                expect(result).to.deep.equal(expected);
+            });
+        });
+    });
+});