Repository: lesshint/lesshint (js)

Issue:
spaceAroundComma linter breaks on multiline definition
For very long definitions, as you might find with gradients, we'd like to be able separate each step on to a new line without the `spaceAroundComma` linter flagging it. Perhaps this needs to be an additional option?

Something like the following should be considered valid, in my opinion:
```less
.thing {
    background: linear-gradient(
        to bottom,
        transparent 0,
        transparent 25%,
        red 25%,
        red 75%,
        transparent 75%,
        transparent 0
    );
}
```

Pull request:
fixes #119: adds allowNewline option to spaceAroundComma
This is my proposal for the spaceAroundComma fix when newlines are used to separate comma-delimited lists. `allowNewline` set to `true` will skip any verification of space around a comma if a newline is detected, based on the `style` option chosen. 

Patch:
diff --git a/lib/config/defaults.json b/lib/config/defaults.json
index ebcbc0d..646252a 100644
--- a/lib/config/defaults.json
+++ b/lib/config/defaults.json
@@ -132,6 +132,7 @@
     },
 
     "spaceAroundComma": {
+        "allowNewline": false,
         "enabled": true,
         "style": "after"
     },
diff --git a/lib/linter.js b/lib/linter.js
index a702239..8c4249f 100644
--- a/lib/linter.js
+++ b/lib/linter.js
@@ -96,6 +96,9 @@ var runChecks = function (ast, fullPath, lines, config) {
     var inlineOptions;
     var linters;
 
+    // tests selectors for pseudo classes/selectors. eg. :not(), :active
+    var rPseudo = /::?[^ ,:.]+/g;
+
     // Freeze the AST so linters won't accidentally change it
     Object.freeze(ast);
 
@@ -133,7 +136,23 @@ var runChecks = function (ast, fullPath, lines, config) {
                 return;
             }
 
+            // if we're dealing with a regular Rule (or other) node, which isn't
+            // an actual Mixin or AtRule, and its selector contains a pseudo
+            // class or selector, then clean up the raws and params properties.
+            // tracking: https://github.com/webschik/postcss-less/issues/56
+            // TODO: remove this when issue resolved
+            if (node.params && rPseudo.test(node.selector)) {
+                delete node.params;
+
+                // this just started showing up in postcss-less@0.14.0. not sure
+                // if it's sticking around, but making sure we're thorough.
+                if (node.raws.params) {
+                    delete node.raws.params;
+                }
+            }
+
             lint = linter.lint.call(linter, options, node);
+
             if (lint) {
                 if (!Array.isArray(lint)) {
                     throw new Error('Linter ' + linter.name + ' must return an array.');
diff --git a/lib/linters/README.md b/lib/linters/README.md
index a957c8a..da26f1f 100644
--- a/lib/linters/README.md
+++ b/lib/linters/README.md
@@ -606,9 +606,10 @@ Option     | Description
 ## spaceAroundComma
 Defines how commas in functions, mixins, etc. should be formatted by a space to aid readability.
 
-Option     | Description
----------- | ----------
-`style`    | `after` (**default**), `before`, `both`, `none`
+Option          | Description
+----------------| ----------
+`allowNewline`  | `false` (**default**), `boolean`
+`style`         | `after` (**default**), `before`, `both`, `none`
 
 ### after
 ```less
@@ -638,6 +639,16 @@ Option     | Description
 }
 ```
 
+### allowNewline : true
+```less
+.foo {
+  font:
+    14px,
+    Roboto,
+    #000;
+}
+```
+
 ## spaceAroundOperator
 Defines how operators (`+`, `-`, `*`, `/`, and `=`) should be formatted by a space to aid readability.
 
diff --git a/lib/linters/important_rule.js b/lib/linters/important_rule.js
index c20f810..b099561 100644
--- a/lib/linters/important_rule.js
+++ b/lib/linters/important_rule.js
@@ -9,10 +9,6 @@ module.exports = {
         var pos = 1;
 
         if (node.important) {
-            if (node.raws.important) {
-                pos = node.raws.important.indexOf('!');
-            }
-
             return [{
                 column: node.source.start.column + node.prop.length + node.raws.between.length + node.value.length + pos,
                 line: node.source.start.line,
diff --git a/lib/linters/qualifying_element.js b/lib/linters/qualifying_element.js
index eec4b27..d8d2301 100644
--- a/lib/linters/qualifying_element.js
+++ b/lib/linters/qualifying_element.js
@@ -26,7 +26,7 @@ module.exports = {
             }
 
             result.endsWith = selector.last.type;
-
+            console.log(selector.nodes);
             if (!result.hasTag) {
                 selector.nodes.forEach(function (element) {
                     if (element.type === 'tag') {
diff --git a/lib/linters/space_around_comma.js b/lib/linters/space_around_comma.js
index c5c2be0..21ae40c 100644
--- a/lib/linters/space_around_comma.js
+++ b/lib/linters/space_around_comma.js
@@ -36,14 +36,28 @@ module.exports = {
             index = child.parent.index(child);
             next = child.parent.nodes[index + 1];
 
+            // the user has opted to ignore decls, etc where newlines are used
+            // to separate comma-delimited lists.
+            if (config.allowNewline) {
+                if (config.style === 'after' && /\n/g.test(next.raws.before)) {
+                    return;
+                }
+
+                if (/\n/g.test(child.raws.before)) {
+                    return;
+                }
+            }
+
             // newlines aren't accounted for. remove em cause we don't count
-            // em. (#209)
+            // em. (#209) users can choose to turn off trailing spaces and choose
+            // to have trailing whitespace after a comma, but before a newline.
+            // it's silly, but it's an available option.
             if (next && next.raws && next.raws.before) {
                 next.raws.before = next.raws.before.replace(/\n/g, '');
             }
 
-            if (child && child.raws && child.raws.after) {
-                child.raws.after = child.raws.after.replace(/\n/g, '');
+            if (child && child.raws && child.raws.before) {
+                child.raws.before = child.raws.before.replace(/\n/g, '');
             }
 
             switch (config.style) {


Test patch:
diff --git a/test/specs/linter.js b/test/specs/linter.js
index e7ad937..df6bc44 100644
--- a/test/specs/linter.js
+++ b/test/specs/linter.js
@@ -366,6 +366,16 @@ describe('linter', function () {
             expect(result).to.deep.equal(expected);
         });
 
+        it('should not report comma spaces for selectors that have pseudos', function () {
+            var source = '.foo,\n.bar:not(.foo){}';
+            var path = 'test.less';
+            var result;
+
+            result = linter.lint(source, path, {});
+
+            expect(result).to.have.length(0);
+        });
+
         it('should load a custom linter (as a require path)', function () {
             var source = '// boo!\n';
             var path = 'test.less';
diff --git a/test/specs/linters/attribute_quotes.js b/test/specs/linters/attribute_quotes.js
index 73df4d6..c307b50 100644
--- a/test/specs/linters/attribute_quotes.js
+++ b/test/specs/linters/attribute_quotes.js
@@ -13,6 +13,16 @@ describe('lesshint', function () {
             });
         });
 
+        it('should bail if the node has no selector', function () {
+            var source = "input[type='text'] { width: 0; }";
+
+            return spec.parse(source, function (ast) {
+                var result = spec.linter.lint({}, ast.root.first.first);
+
+                expect(result).to.be.undefined;
+            });
+        });
+
         it('should allow single quotes', function () {
             var source = "input[type='text'] {}";
 
diff --git a/test/specs/linters/qualifying_element.js b/test/specs/linters/qualifying_element.js
index b5e81ed..5712f69 100644
--- a/test/specs/linters/qualifying_element.js
+++ b/test/specs/linters/qualifying_element.js
@@ -2,6 +2,7 @@
 
 var expect = require('chai').expect;
 var spec = require('../util.js').setup();
+var parser = require('postcss-selector-parser');
 
 describe('lesshint', function () {
     describe('#qualifyingElement()', function () {
@@ -146,5 +147,56 @@ describe('lesshint', function () {
                 expect(result).to.be.undefined;
             });
         });
+
+        it('should inspect the parent selector', function () {
+            var source = '.a { &.active { color: red; } }';
+
+            return spec.parse(source, function (ast) {
+                var node = ast.root.first;
+                var expected = { startsWith: 'class', endsWith: 'class', hasTag: false };
+                var result;
+
+                parser(function (selectors) {
+                    node.selectorAst = selectors;
+                    result = spec.linter.inspectParent(node.first);
+                    expect(result).to.deep.equal(expected);
+
+                }).process(node.selector);
+            });
+        });
+
+        it('should inspect the parent selector, recognize tag', function () {
+            var source = 'a { &.active { color: red; } }';
+
+            return spec.parse(source, function (ast) {
+                var node = ast.root.first;
+                var expected = { startsWith: 'tag', endsWith: 'tag', hasTag: true };
+                var result;
+
+                parser(function (selectors) {
+                    node.selectorAst = selectors;
+                    result = spec.linter.inspectParent(node.first);
+                    expect(result).to.deep.equal(expected);
+
+                }).process(node.selector);
+            });
+        });
+
+        it('should inspect the parent selector, recognize tag and class', function () {
+            var source = '.b a { &.active { color: red; } }';
+
+            return spec.parse(source, function (ast) {
+                var node = ast.root.first;
+                var expected = { startsWith: 'class', endsWith: 'tag', hasTag: true };
+                var result;
+
+                parser(function (selectors) {
+                    node.selectorAst = selectors;
+                    result = spec.linter.inspectParent(node.first);
+                    expect(result).to.deep.equal(expected);
+
+                }).process(node.selector);
+            });
+        });
     });
 });
diff --git a/test/specs/linters/space_around_comma.js b/test/specs/linters/space_around_comma.js
index c1a3061..1b25579 100644
--- a/test/specs/linters/space_around_comma.js
+++ b/test/specs/linters/space_around_comma.js
@@ -72,6 +72,16 @@ describe('lesshint', function () {
                 });
             });
 
+            it('should account for multiline rules with commas containing pseudo classes', function () {
+                var source = '.test1,\n.test2:not(.test3),\n.test3:not(.test2) {\n    width: 100%;\n}';
+
+                return spec.parse(source, function (ast) {
+                    var result = spec.linter.lint(options, ast.root.first);
+
+                    expect(result).to.be.undefined;
+                });
+            });
+
             it('should not allow missing space after comma in mixins', function () {
                 var source = '.mixin(@margin,@padding) {}';
                 var expected = [{
@@ -85,6 +95,41 @@ describe('lesshint', function () {
                     expect(result).to.deep.equal(expected);
                 });
             });
+
+            it('should not allow multiline comma lists when allowNewline is false', function () {
+                var source = 'font: 14px,\n    Roboto,\n    #000000;';
+                var expected = [
+                    {
+                        column: 11,
+                        message: 'Commas should be followed by one space.'
+                    },
+                    {
+                        column: 17,
+                        message: 'Commas should be followed by one space.'
+                    }
+                ];
+
+                options.allowNewline = false;
+
+                return spec.parse(source, function (ast) {
+                    var result = spec.linter.lint(options, ast.root.first);
+
+                    expect(result).to.deep.equal(expected);
+                });
+            });
+
+            it('should allow multiline comma lists when allowNewline is true', function () {
+                var source = 'font: 14px,\n    Roboto,\n    #000000;';
+
+                options.allowNewline = true;
+
+                return spec.parse(source, function (ast) {
+                    var result = spec.linter.lint(options, ast.root.first);
+
+                    expect(result).to.be.undefined;
+                });
+            });
+
         }); // "after"
 
         describe('when "style" is "before"', function () {
@@ -147,6 +192,18 @@ describe('lesshint', function () {
                     expect(result).to.deep.equal(expected);
                 });
             });
+
+            it('should allow multiline comma lists when allowNewline is true', function () {
+                var source = 'font: 14px\n,    Roboto\n,    #000000;';
+
+                options.allowNewline = true;
+
+                return spec.parse(source, function (ast) {
+                    var result = spec.linter.lint(options, ast.root.first);
+
+                    expect(result).to.be.undefined;
+                });
+            });
         }); // "before"
 
         describe('when "style" is "both"', function () {
diff --git a/test/specs/linters/url_quotes.js b/test/specs/linters/url_quotes.js
index d1a1ace..7e7fa5c 100644
--- a/test/specs/linters/url_quotes.js
+++ b/test/specs/linters/url_quotes.js
@@ -21,8 +21,28 @@ describe('lesshint', function () {
             });
         });
 
+        it('should not check nodes without params', function () {
+            var source = '.foo { @bar }';
+
+            return spec.parse(source, function (ast) {
+                var result = spec.linter.lint({}, ast.root.first.first);
+
+                expect(result).to.be.undefined;
+            });
+        });
+
+        it('should not check nodes without values', function () {
+            var source = '.foo { @bar() }';
+
+            return spec.parse(source, function (ast) {
+                var result = spec.linter.lint({}, ast.root.first.first);
+
+                expect(result).to.be.undefined;
+            });
+        });
+
         it('should allow single quotes', function () {
-            var source = ".foo { background-image: url('img/image.jpg'); }";
+            var source = '.foo { background-image: url(\'img/image.jpg\'); }';
 
             return spec.parse(source, function (ast) {
                 var result = spec.linter.lint({}, ast.root.first.first);
diff --git a/test/specs/util.js b/test/specs/util.js
index df1acb4..896a306 100644
--- a/test/specs/util.js
+++ b/test/specs/util.js
@@ -8,6 +8,9 @@ module.exports = {
         var filename = '../../lib/linters/';
         var linter;
 
+        // tests selectors for pseudo classes/selectors. eg. :not(), :active
+        var rPseudo = /::?[^ ,:.]+/g;
+
         // slightly evil, but it's OK since this is just for specs
         // nodejs caches module.parent as the first module to require it,
         // which in the case of specs, is mocha.
@@ -27,6 +30,25 @@ module.exports = {
             parser: getParser,
             parse: function (source, callback) {
                 return getParser(source).then(function (ast) {
+                    // if we're dealing with a regular Rule (or other) node, which isn't
+                    // an actual Mixin or AtRule, and its selector contains a pseudo
+                    // class or selector, then clean up the raws and params properties.
+                    // if we don't have this here, then the tests never get the same
+                    // modified nodes.
+                    // tracking: https://github.com/webschik/postcss-less/issues/56
+                    // TODO: remove this when issue resolved
+                    ast.root.walk(function (node) {
+                        if (node.params && rPseudo.test(node.selector)) {
+                            delete node.params;
+
+                            // this just started showing up in postcss-less@0.14.0. not sure
+                            // if it's sticking around, but making sure we're thorough.
+                            if (node.raws.params) {
+                                delete node.raws.params;
+                            }
+                        }
+                    });
+
                     callback && callback(ast);
                 });
             }