Repository: lesshint/lesshint
Pull request #77 (2015-10-07 14:05:08)

propertyUnits: valid, invalid specification. addresses #36
propertyUnits: valid, invalid specification. addresses #36

kokarn: This looks good and because you are so quick with the PRs i'd like to merge.
Mind having a look on the failing tests?
shellscape: @kokarn no idea what's up with that. they were passing locally. 
shellscape: updating to gonzales-pe @ 3.0.0-beta was the cause of some of the errors. I reverted the commit.
kokarn: Can you update the PR?
shellscape: @kokarn in what way? specs are passing locally. I don't know why either services are failing.
kokarn: Hmm, I guess we need to specify the correct version of gonzales-pe. Wouldn't the tests failing on Travis and Appveyor mean that it's using the wrong version?
shellscape: I reverted the commit that was using 3.0.0-beta, and it's back to 3.0.0-31, which is what master is using.
kokarn: Hmm, this is weird. There is only the one commit for this PR so it shouldn't fail :/
shellscape: For whatever reason, I had to blow away and reclone to start seeing the failing specs. Anyhow, it's due to the propertyUnits change. There's one more error being thrown on the checkDirectory specs:

```
{ column: 19,
      line: 2,
      linter: 'propertyUnits',
      message: 'Unit "px" is not allowed for "margin-bottom".',
      file: 'vendor.less',
      source: '    margin-bottom:10px;',
      severity: 'warning' }
```

I ran into this yesterday with our internal linting. Because propertyUnits is on by default, and by default there are no units specified, **all** units are invalid.

I'm thinking this is a candidate for a disabled-by-default linter. Thoughts?
kokarn: I'd say the defaults are messed up, not that it should be disabled.

A little googling gave me [this list](http://www.w3.org/TR/css3-values/)
`em`, `ex`, `ch`, `rem`
`vw`, `vh`, `vmin`, `vmax`
`cm`, `mm`, `q`, `in`, `pt`, `pc`, `px`
`deg`, `grad`, `rad`, `turn`
`s`, `ms`
`Hz`, `kHz`
`dpi`, `dpcm`, `dppx`

My suggestion is to enable `deg`, `rad`, `s`, `dpi`, `dppx`, `em`, `rem`, `vw`, `vh` and `px` and disable the rest.
shellscape: Do any of our other linters follow that pattern? (Of having a linter option populated with defaults)
shellscape: Ah ignore that. Just found this in defaults.json:

```
    "propertyUnits": {
        "enabled": true,
        "global": [
            "ch", "em", "ex", "rem",                 // Font-relative lengths
            "cm", "in", "mm", "pc", "pt", "px", "q", // Absolute lengths
            "vh", "vw", "vmin", "vmax",              // Viewport-percentage lengths
            "deg", "grad", "rad", "turn",            // Angle
            "ms", "s",                               // Duration
            "Hz", "kHz",                             // Frequency
            "dpi", "dpcm", "dppx",                   // Resolution
            "%"                                      // Other
        ],
        "properties": {}
    },
```

So I missed the defaults config file. Fixing that...
kokarn: Looks like a missed `%` as well :)

Patch:
diff --git a/lib/config/defaults.json b/lib/config/defaults.json
index 7726b3a..1b44157 100644
--- a/lib/config/defaults.json
+++ b/lib/config/defaults.json
@@ -73,7 +73,7 @@
 
     "propertyUnits": {
         "enabled": true,
-        "global": [
+        "valid": [
             "ch", "em", "ex", "rem",                 // Font-relative lengths
             "cm", "in", "mm", "pc", "pt", "px", "q", // Absolute lengths
             "vh", "vw", "vmin", "vmax",              // Viewport-percentage lengths
diff --git a/lib/linters/README.md b/lib/linters/README.md
index af523a0..fa3bc4f 100644
--- a/lib/linters/README.md
+++ b/lib/linters/README.md
@@ -334,16 +334,18 @@ and the `properties` option can be used to fine tune units for each property.
 
 *Note: Shorthands are not supported by the `properties` option. For example, to specify units for `margin`, all margin-* properties must be specified.*
 
-Option       | Description
------------- | ----------
-`global`     | Allowed units (by default all units are allowed)
-`properties` | Object with property names and allowed units (empty by default)
+Option        | Description
+------------- | ----------
+`valid`       | Allowed units (by default all units are allowed)
+`invalid`     | Units not allowed under any circumstances. Overrides all other options.
+`properties`  | Object with property names and allowed units (empty by default)
 
 ```js
 "propertyUnits": {
-    "global": ["rem", "vw"], // These units are allowed for all properties
+    "valid": ["rem", "vw"], // These units are allowed for all properties
+    "invalid": ["pt"],      // The 'pt' unit is not allowed under any circumstances
     "properties": {
-        "line-height": [] // No units are allowed for line-height
+        "line-height": []   // No units are allowed for line-height
     }
 }
 ```
diff --git a/lib/linters/property_units.js b/lib/linters/property_units.js
index 688321b..ddecfac 100644
--- a/lib/linters/property_units.js
+++ b/lib/linters/property_units.js
@@ -18,18 +18,27 @@ module.exports = {
         property = node.first('property').first('ident').content;
         value = node.first('value');
 
+        // sanity checking
+        config.valid = Array.isArray(config.valid) ? config.valid : [];
+        config.invalid = Array.isArray(config.invalid) ? config.invalid : [];
+        config.properties = config.properties || {};
+
         value.forEach('dimension', function (element) {
             var allowed = config.properties[property];
             var unit = element.first('ident').content;
 
-            // Check if the unit is allowed for this property
-            if (Array.isArray(allowed) && allowed.indexOf(unit) !== -1) {
-                return;
-            }
+            // if this unit is marked invalid, then the other checks are moot
+            if (config.invalid.indexOf(unit) === -1) {
+
+                // Check if the unit is allowed for this property
+                if (Array.isArray(allowed) && allowed.indexOf(unit) !== -1) {
+                    return;
+                }
 
-            // Unit allowed, bail
-            if (!allowed && config.global.indexOf(unit) !== -1) {
-                return;
+                // Unit allowed, bail
+                if (!allowed && config.valid.indexOf(unit) !== -1) {
+                    return;
+                }
             }
 
             results.push({
@@ -40,12 +49,15 @@ module.exports = {
         });
 
         // Check if percentages are allowed
-        if (config.global.indexOf('%') === -1) {
+        if (config.valid.indexOf('%') === -1) {
             value.forEach('percentage', function (element) {
-                // Check if the percentages are allowed for this property
-                if (config.properties[property]) {
-                    if (config.properties[property].indexOf('%') !== -1) {
-                        return;
+
+                if (config.invalid.indexOf('%') === -1) {
+                    // Check if the percentages are allowed for this property
+                    if (config.properties[property]) {
+                        if (config.properties[property].indexOf('%') !== -1) {
+                            return;
+                        }
                     }
                 }
 


Test patch:
diff --git a/test/specs/linters/property_units.js b/test/specs/linters/property_units.js
index dbb6316..8b52fb1 100644
--- a/test/specs/linters/property_units.js
+++ b/test/specs/linters/property_units.js
@@ -7,14 +7,14 @@ var undefined;
 
 describe('lesshint', function () {
     describe('#propertyUnits()', function () {
-        it('should allow allowed global unit', function () {
+        it('should allow allowed valid unit', function () {
             var source = '.foo { font-size: 1rem; }';
             var ast;
 
             var options = {
                 propertyUnits: {
                     enabled: true,
-                    global: ['rem'],
+                    valid: ['rem'],
                     properties: {}
                 }
             };
@@ -25,14 +25,14 @@ describe('lesshint', function () {
             assert.strictEqual(undefined, lint(options, ast));
         });
 
-        it('should allow allowed property unit', function () {
+        it('should allow valid property unit', function () {
             var source = '.foo { font-size: 1rem; }';
             var ast;
 
             var options = {
                 propertyUnits: {
                     enabled: true,
-                    global: [],
+                    valid: [],
                     properties: {
                         'font-size': ['rem']
                     }
@@ -45,7 +45,7 @@ describe('lesshint', function () {
             assert.strictEqual(undefined, lint(options, ast));
         });
 
-        it('should not allow disallowed global unit', function () {
+        it('should not allow an unspecified unit', function () {
             var source = '.foo { font-size: 1rem; }';
             var ast;
 
@@ -59,7 +59,7 @@ describe('lesshint', function () {
             var options = {
                 propertyUnits: {
                     enabled: true,
-                    global: ['px'],
+                    valid: ['px'],
                     properties: {}
                 }
             };
@@ -72,7 +72,7 @@ describe('lesshint', function () {
             assert.deepEqual(actual, expected);
         });
 
-        it('should not allow disallowed property unit', function () {
+        it('should not allow an unspecified property unit', function () {
             var source = '.foo { font-size: 1rem; }';
             var ast;
 
@@ -86,7 +86,7 @@ describe('lesshint', function () {
             var options = {
                 propertyUnits: {
                     enabled: true,
-                    global: [],
+                    valid: [],
                     properties: {
                         'font-size': ['px']
                     }
@@ -101,7 +101,7 @@ describe('lesshint', function () {
             assert.deepEqual(actual, expected);
         });
 
-        it('should not allow any units when no global units are passed', function () {
+        it('should not allow any units when no valid units are passed', function () {
             var source = '.foo { line-height: 24px; }';
             var ast;
 
@@ -115,7 +115,7 @@ describe('lesshint', function () {
             var options = {
                 propertyUnits: {
                     enabled: true,
-                    global: [],
+                    valid: [],
                     properties: {}
                 }
             };
@@ -142,7 +142,7 @@ describe('lesshint', function () {
             var options = {
                 propertyUnits: {
                     enabled: true,
-                    global: ['px'],
+                    valid: ['px'],
                     properties: {
                         'line-height': []
                     }
@@ -157,14 +157,14 @@ describe('lesshint', function () {
             assert.deepEqual(actual, expected);
         });
 
-        it('should allow percentages when set as a allowed global unit', function () {
+        it('should allow percentages when set as a valid unit', function () {
             var source = '.foo { font-size: 100%; }';
             var ast;
 
             var options = {
                 propertyUnits: {
                     enabled: true,
-                    global: ['%'],
+                    valid: ['%'],
                     properties: {}
                 }
             };
@@ -175,14 +175,14 @@ describe('lesshint', function () {
             assert.strictEqual(undefined, lint(options, ast));
         });
 
-        it('should allow percentages when set as a allowed property unit', function () {
+        it('should allow percentages when set as a valid property unit', function () {
             var source = '.foo { font-size: 100%; }';
             var ast;
 
             var options = {
                 propertyUnits: {
                     enabled: true,
-                    global: [],
+                    valid: [],
                     properties: {
                         'font-size': ['%']
                     }
@@ -195,7 +195,7 @@ describe('lesshint', function () {
             assert.strictEqual(undefined, lint(options, ast));
         });
 
-        it('should not allow percentages when set as a disallowed global unit', function () {
+        it('should not allow percentages when set as an invalid unit', function () {
             var source = '.foo { font-size: 100%; }';
             var ast;
 
@@ -209,7 +209,7 @@ describe('lesshint', function () {
             var options = {
                 propertyUnits: {
                     enabled: true,
-                    global: ['px'],
+                    valid: ['px'],
                     properties: {}
                 }
             };
@@ -222,7 +222,7 @@ describe('lesshint', function () {
             assert.deepEqual(actual, expected);
         });
 
-        it('should not allow percentages when set as a disallowed property unit', function () {
+        it('should not allow percentages when set as an invalid property unit', function () {
             var source = '.foo { font-size: 100%; }';
             var ast;
 
@@ -236,7 +236,91 @@ describe('lesshint', function () {
             var options = {
                 propertyUnits: {
                     enabled: true,
-                    global: [],
+                    valid: [],
+                    properties: {
+                        'font-size': ['px']
+                    }
+                }
+            };
+
+            ast = parseAST(source);
+            ast = ast.first().first('block').first('declaration');
+
+            actual = lint(options, ast);
+
+            assert.deepEqual(actual, expected);
+        });
+
+        it('should not allow an invalid unit', function () {
+            var source = '.foo { font-size: 1px; }';
+            var ast;
+
+            var expected = [{
+                column: 19,
+                line: 1,
+                linter: 'propertyUnits',
+                message: 'Unit "px" is not allowed for "font-size".'
+            }];
+
+            var options = {
+                propertyUnits: {
+                    enabled: true,
+                    invalid: ['px'],
+                    properties: {}
+                }
+            };
+
+            ast = parseAST(source);
+            ast = ast.first().first('block').first('declaration');
+
+            actual = lint(options, ast);
+
+            assert.deepEqual(actual, expected);
+        });
+
+        it('should not allow an invalid unit that overrides a valid unit', function () {
+            var source = '.foo { font-size: 1px; }';
+            var ast;
+
+            var expected = [{
+                column: 19,
+                line: 1,
+                linter: 'propertyUnits',
+                message: 'Unit "px" is not allowed for "font-size".'
+            }];
+
+            var options = {
+                propertyUnits: {
+                    enabled: true,
+                    valid: ['px'],
+                    invalid: ['px'],
+                    properties: {}
+                }
+            };
+
+            ast = parseAST(source);
+            ast = ast.first().first('block').first('declaration');
+
+            actual = lint(options, ast);
+
+            assert.deepEqual(actual, expected);
+        });
+
+        it('should not allow an invalid unit that has been specified valid for a property', function () {
+            var source = '.foo { font-size: 1px; }';
+            var ast;
+
+            var expected = [{
+                column: 19,
+                line: 1,
+                linter: 'propertyUnits',
+                message: 'Unit "px" is not allowed for "font-size".'
+            }];
+
+            var options = {
+                propertyUnits: {
+                    enabled: true,
+                    invalid: ['px'],
                     properties: {
                         'font-size': ['px']
                     }