Repository: lesshint/lesshint (js)

Issue:
False positive with "singleLinePerProperty"
`singleLineProperty` is returning a false positive for inline comments.

Valid CSS

```LESS
.foo {
  background-color: #000;
  color: #000; // black on black
}
```

Shouldn't error
```
test.less: line 3, col 5, singleLinePerProperty: Each property should be on it's own line.
```

Please add support for inline comments

Pull request:
feat(singleLinePerProperty): allow inline comments
Closes #51 

Patch:
diff --git a/lib/linters/single_line_per_property.js b/lib/linters/single_line_per_property.js
index 9223911..f762e17 100644
--- a/lib/linters/single_line_per_property.js
+++ b/lib/linters/single_line_per_property.js
@@ -1,5 +1,60 @@
 'use strict';
 
+var isPreviousValid = function (node, element, index) {
+    var prev = node.get(index - 1);
+
+    if (!prev) {
+        // no previous sibling, assume beginning of block without new line, mark invalid
+        return false;
+    } else if (prev.type === 'space' && !prev.toString().match(/\n/)) {
+        // previous sibling not new line
+        return false;
+    } else {
+        return true;
+    }
+};
+
+var isNextValid = function (node, element, index) {
+    var next = node.get(index + 2); // We need to move two nodes since the first one is the declaration delimiter (;)
+    var isValid;
+
+    if (!next) {
+        // no next sibling, assume end of block without new line, mark invalid
+        isValid = false;
+    } else {
+        switch (next.type) {
+            case 'space':
+                if (next.content.match(/\n/)) {
+                    isValid = true;
+                } else {
+                    // sibling is not new line
+                    var afterNext = node.get(index + 3);
+
+                    if (!afterNext) {
+                        // assume end of block without new line, mark invalid
+                        isValid = false;
+                    } else {
+                        // only valid if followed by comment
+                        if (afterNext.type === 'multilineComment' || afterNext.type === 'singlelineComment') {
+                            isValid = true;
+                        } else {
+                            isValid = false;
+                        }
+                    }
+                }
+                break;
+            case 'singlelineComment': // single-line comment butt up against declaration
+                isValid = true;
+                break;
+            case 'multilineComment': // multi-line comment butt up against declaration
+                isValid = true;
+                break;
+        }
+    }
+
+    return isValid;
+};
+
 module.exports = function (options) {
     var config = options.config;
     var node = options.node;
@@ -16,15 +71,15 @@ module.exports = function (options) {
     }
 
     node.forEach('declaration', function (element, index) {
-        var prev = node.get(index - 1);
-        var next = node.get(index + 2); // We need to move two nodes since the first one is the declaration delimiter (;)
+        var previousValid = isPreviousValid(node, element, index);
+        var nextValid = isNextValid(node, element, index);
 
-        // If no sibling nodes were found, assume there's no new line
-        if ((!prev || !next) || (prev.content.indexOf('\n') === -1 || next.content.indexOf('\n') === -1)) {
+        // BOTH previous and next must be valid
+        if (!(previousValid && nextValid)) {
             results.push({
                 column: element.start.column,
                 line: element.start.line,
-                message: "Each property should be on it's own line."
+                message: 'Each property should be on its own line.'
             });
         }
     });


Test patch:
diff --git a/test/specs/linters/single_line_per_property.js b/test/specs/linters/single_line_per_property.js
index 3b3ddf6..2151b1f 100644
--- a/test/specs/linters/single_line_per_property.js
+++ b/test/specs/linters/single_line_per_property.js
@@ -3,6 +3,7 @@ var assert = require('assert');
 describe('lesshint', function () {
     var linter = require('../../../lib/linter');
     var singleLinePerProperty = require('../../../lib/linters/single_line_per_property');
+    var errorMessage = 'Each property should be on its own line.';
 
     describe('#singleLinePerProperty()', function () {
         it('should allow properties on separate lines', function () {
@@ -33,13 +34,73 @@ describe('lesshint', function () {
                 column: 2,
                 line: 2,
                 linter: 'singleLinePerProperty',
-                message: "Each property should be on it's own line."
+                message: errorMessage
             },
             {
                 column: 14,
                 line: 2,
                 linter: 'singleLinePerProperty',
-                message: "Each property should be on it's own line."
+                message: errorMessage
+            }];
+
+            var options = {
+                singleLinePerProperty: {
+                    enabled: true
+                }
+            };
+
+            ast = linter.parseAST(source);
+            ast = ast.first('ruleset').first('block');
+
+            actual = singleLinePerProperty({
+                config: options,
+                node: ast,
+                path: 'test.less'
+            });
+
+            assert.deepEqual(actual, expected);
+        });
+
+        it('should not allow sole property in single-line block (spaces between braces)', function () {
+            var source = '.foo { color: red; }';
+            var actual;
+            var ast;
+
+            var expected = [{
+                column: 8,
+                line: 1,
+                linter: 'singleLinePerProperty',
+                message: errorMessage
+            }];
+
+            var options = {
+                singleLinePerProperty: {
+                    enabled: true
+                }
+            };
+
+            ast = linter.parseAST(source);
+            ast = ast.first('ruleset').first('block');
+
+            actual = singleLinePerProperty({
+                config: options,
+                node: ast,
+                path: 'test.less'
+            });
+
+            assert.deepEqual(actual, expected);
+        });
+
+        it('should not allow sole property in single-line block (no spaced between braces)', function () {
+            var source = '.foo {color: red;}';
+            var actual;
+            var ast;
+
+            var expected = [{
+                column: 7,
+                line: 1,
+                linter: 'singleLinePerProperty',
+                message: errorMessage
             }];
 
             var options = {
@@ -69,7 +130,7 @@ describe('lesshint', function () {
                 column: 8,
                 line: 1,
                 linter: 'singleLinePerProperty',
-                message: "Each property should be on it's own line."
+                message: errorMessage
             }];
 
             var options = {
@@ -99,7 +160,7 @@ describe('lesshint', function () {
                 column: 2,
                 line: 3,
                 linter: 'singleLinePerProperty',
-                message: "Each property should be on it's own line."
+                message: errorMessage
             }];
 
             var options = {
@@ -153,5 +214,151 @@ describe('lesshint', function () {
                 node: ast
             }));
         });
+
+        it('should return null with single-line comment after property definition', function () {
+            var source = '.foo {\n color: red;\n margin-right: 10px; // inline comment\n}';
+            var options = {
+                singleLinePerProperty: {
+                    enabled: true
+                }
+            };
+            var ast;
+
+            ast = linter.parseAST(source);
+            ast = ast.first('ruleset').first('block');
+
+            assert.equal(null, singleLinePerProperty({
+                config: options,
+                node: ast,
+                path: 'test.less'
+            }));
+        });
+
+        it('should return null with single-line comment immediately after property definition', function () {
+            var source = '.foo {\n color: red;\n margin-right: 10px;// inline comment\n}';
+            var options = {
+                singleLinePerProperty: {
+                    enabled: true
+                }
+            };
+            var ast;
+
+            ast = linter.parseAST(source);
+            ast = ast.first('ruleset').first('block');
+
+            assert.equal(null, singleLinePerProperty({
+                config: options,
+                node: ast,
+                path: 'test.less'
+            }));
+        });
+
+        it('should return null with one-line, multi-line comment after property definition', function () {
+            var source = '.foo {\n color: red;\n margin-right: 10px; /* inline comment */\n}';
+            var options = {
+                singleLinePerProperty: {
+                    enabled: true
+                }
+            };
+            var ast;
+
+            ast = linter.parseAST(source);
+            ast = ast.first('ruleset').first('block');
+
+            assert.equal(null, singleLinePerProperty({
+                config: options,
+                node: ast,
+                path: 'test.less'
+            }));
+        });
+
+        it('should return null with one-line, multi-line comment immediately after property definition', function () {
+            var source = '.foo {\n color: red;\n margin-right: 10px;/* inline comment */\n}';
+            var options = {
+                singleLinePerProperty: {
+                    enabled: true
+                }
+            };
+            var ast;
+
+            ast = linter.parseAST(source);
+            ast = ast.first('ruleset').first('block');
+
+            assert.equal(null, singleLinePerProperty({
+                config: options,
+                node: ast,
+                path: 'test.less'
+            }));
+        });
+
+        it('should not allow single-line, inline comment after multiple definitions in same line', function () {
+            var source = '.foo {\n color: red; margin-right: 10px; // inline comment\n}';
+            var actual;
+            var ast;
+
+            var expected = [{
+                column: 2,
+                line: 2,
+                linter: 'singleLinePerProperty',
+                message: errorMessage
+            }, {
+                column: 14,
+                line: 2,
+                linter: 'singleLinePerProperty',
+                message: errorMessage
+            }];
+
+            var options = {
+                singleLinePerProperty: {
+                    enabled: true
+                }
+            };
+
+            ast = linter.parseAST(source);
+            ast = ast.first('ruleset').first('block');
+
+            actual = singleLinePerProperty({
+                config: options,
+                node: ast,
+                path: 'test.less'
+            });
+
+            assert.deepEqual(actual, expected);
+        });
+
+        it('should not allow one-line, multi-line comment after multiple definitions in same line', function () {
+            var source = '.foo {\n color: red; margin-right: 10px; /* inline comment */\n}';
+            var actual;
+            var ast;
+
+            var expected = [{
+                column: 2,
+                line: 2,
+                linter: 'singleLinePerProperty',
+                message: errorMessage
+            }, {
+                column: 14,
+                line: 2,
+                linter: 'singleLinePerProperty',
+                message: errorMessage
+            }];
+
+            var options = {
+                singleLinePerProperty: {
+                    enabled: true
+                }
+            };
+
+            ast = linter.parseAST(source);
+            ast = ast.first('ruleset').first('block');
+
+            actual = singleLinePerProperty({
+                config: options,
+                node: ast,
+                path: 'test.less'
+            });
+
+            assert.deepEqual(actual, expected);
+        });
     });
 });