Repository: lesshint/lesshint
Pull request #234 (2016-08-02 07:00:37)

Add the Max Character Limit linter
Somehow linter.js is not running my new linter after merging in change #216. However, this linter could work if it could run. I am looking into why it doesn't run. 

kenliao94: Not ready for review
shellscape: The first step would be to create specs for this linter. I would start debugging there, first.
jwilsson: Hi,
Looks good so far! Just two things for now:

1. You'll need to add the linter to the array at the top of `linter.js` for it to be called.
2. I think you'll need to merge in the latest `master` so we don't lose any changes.
kenliao94: Set the limit to 100 and put that in the default linter array in linter.js. Thanks for the help @jwilsson :)
kenliao94: Is there a way to locally run those build checks? 
jwilsson: @kenliao94 I think you'll need to run `git fetch` first and then merge.

You can run `npm test` locally to make sure everything passes :)
kenliao94: Thanks @jwilsson :) But I have ran `git fetch upstream/master` then `git merge upstream/master` it said my local master is already up-to-date with the upstream/master. I ran `git log` and the history shows I have merged in the latest commit on master:

commit 92d1e3610a40ff8de800b8c120833ab510edb5f0                                                                                                              
Author: Jonathan Wilsson <jonathan.wilsson@gmail.com>                                                                                                           
Date:   Sun Jul 31 16:54:24 2016 +0200                                                                                                              
2.0.2  

Is there any changes you want me to merge in this pull request? 
kenliao94: Sorry @jwilsson , this is my first time making a PR to an open source project. Do you want me to merge in other people's changes? Thanks for your patience.
jwilsson: @kenliao94 No worries, I'm happy to help :)

This sounds really strange, the thing I'm hung up about is the removal of
```
"allowNewline": false, 
```

in `lib/config/defaults.json` which shouldn't be shown as deleted with latest master but we can take care of that when it's time to merge.
shellscape: Maybe a rebase will fix it?
kenliao94: Hi @shellscape , I tried 
`git rebase upstream/master`
Still didn't see the changes @jwilsson mentioned. That's weird. Here's what I got after running 
`git remote -v` 

`
origin  https://github.com/kenliao94/lesshint (fetch)                                                                                                                      
origin  https://github.com/kenliao94/lesshint (push)
upstream        https://github.com/lesshint/lesshint (fetch)
upstream        https://github.com/lesshint/lesshint (push) 
`

Also, it is failing the coverage test, do I add testcases to increase that coverage?

Thanks!
jwilsson: @kenliao94 I'm still hoping that it'll sort itself out. 

Some tests will be needed, yes. That should solve the decrease in coverage automatically.
kenliao94: Still need to look into why my linter under these testcases always return empty array but ran correctly from terminal.
kenliao94: Also need to add a testcase for multiple lines exceeding character limit.
kenliao94: Fixed test issue & added one more testcase. If all checks pass, ready for review and merge
kenliao94: @shellscape  & @jwilsson Thanks for all your help!
jwilsson: Awesome, I'm good! 🎉 

And thanks for fixing the `spaceAroundComma` setting!

Let's see if anyone else has any comments before merging.

Patch:
diff --git a/lib/config/defaults.json b/lib/config/defaults.json
index 646252a..ca9b128 100644
--- a/lib/config/defaults.json
+++ b/lib/config/defaults.json
@@ -41,6 +41,11 @@
         "enabled": true
     },
 
+    "maxCharPerLine" : {
+        "enabled": true,
+        "limit" : 100
+    },
+
     "hexLength": {
         "enabled": true,
         "style": "long"
diff --git a/lib/linter.js b/lib/linter.js
index c3098ca..2ad5f31 100644
--- a/lib/linter.js
+++ b/lib/linter.js
@@ -39,7 +39,8 @@ var defaultLinters = [
     require('./linters/trailing_whitespace'),
     require('./linters/url_format'),
     require('./linters/url_quotes'),
-    require('./linters/zero_unit')
+    require('./linters/zero_unit'),
+    require('./linters/max_char_per_line')
 ];
 
 var parseRules = function (node) {
diff --git a/lib/linters/max_char_per_line.js b/lib/linters/max_char_per_line.js
new file mode 100644
index 0000000..d7bbe0a
--- /dev/null
+++ b/lib/linters/max_char_per_line.js
@@ -0,0 +1,37 @@
+'use strict';
+
+var util = require('util');
+
+module.exports = {
+    name: 'maxCharPerLine',
+    nodeTypes: ['root'],
+    message: 'Line should not exceed %d characters, %d found.',
+
+    lint: function maxCharPerLineLinter (config, node) {
+        var charLimit = config.limit;
+        var charCount;
+        var lineArray = node.source.input.css.split('\n');
+        var line;
+        var lineNumberArr = [];
+        var resultSet = [];
+        var i;
+
+        for (line = 0; line < lineArray.length; line++) {
+            charCount = lineArray[line].length;
+            if (charCount > charLimit) {
+                lineNumberArr.push({
+                    lineNumber: line + 1,
+                    charCount: charCount,
+                });
+            }
+        }
+        for (i = 0; i < lineNumberArr.length; i++) {
+            resultSet.push({
+                column: 0,
+                line: lineNumberArr[i].lineNumber,
+                message: util.format(this.message, charLimit, lineNumberArr[i].charCount)
+            });
+        }
+        return (resultSet.length > 0) ? resultSet : undefined;
+    }
+};


Test patch:
diff --git a/test/specs/linters/max_char_per_line.js b/test/specs/linters/max_char_per_line.js
new file mode 100644
index 0000000..2fd9463
--- /dev/null
+++ b/test/specs/linters/max_char_per_line.js
@@ -0,0 +1,83 @@
+'use strict';
+
+var expect = require('chai').expect;
+var spec = require('../util.js').setup();
+
+describe('lesshint', function () {
+    describe('#maxCharPerLine()', function () {
+        it('should have the proper node types', function () {
+            var source = '.foo { }';
+
+            return spec.parse(source, function (ast) {
+                expect(spec.linter.nodeTypes).to.include(ast.root.type);
+            });
+        });
+
+        it('should allow files which do not have any lines exceed 10 characters', function () {
+            var source = '.foo {}';
+            var options = {
+                limit: 10
+            };
+
+            return spec.parse(source, function (ast) {
+            // Set to the limit to 10 characters to simplify test source
+                var result = spec.linter.lint(options, ast.root);
+
+                expect(result).to.be.undefined;
+            });
+        });
+
+        it('should not allow files which have any lines that exceed 10 characters', function () {
+            var source = '.foofoofoofoo { }';
+            var expected = [{
+                column: 0,
+                line: 1,
+                message: 'Line should not exceed 10 characters, 17 found.'
+            }];
+            var options = {
+                limit: 10
+            };
+
+            return spec.parse(source, function (ast) {
+                var result = spec.linter.lint(options, ast.root);
+
+                expect(result).to.deep.equal(expected);
+            });
+        });
+
+        it('should report multiple errors if there are multiple lines exceed characters limit', function () {
+            var source = '.foofoofoofoo { }\n.looloolooloo { }';
+            var expected = [
+                {
+                    column: 0,
+                    line: 1,
+                    message: 'Line should not exceed 10 characters, 17 found.'
+                },
+                {
+                    column: 0,
+                    line: 2,
+                    message: 'Line should not exceed 10 characters, 17 found.'
+                }
+            ];
+            var options = {
+                limit: 10
+            };
+
+            return spec.parse(source, function (ast) {
+                var result = spec.linter.lint(options, ast.root);
+
+                expect(result).to.deep.equal(expected);
+            });
+        });
+
+        it('should ignore empty files', function () {
+            var source = '';
+
+            return spec.parse(source, function (ast) {
+                var result = spec.linter.lint({}, ast.root);
+
+                expect(result).to.be.undefined;
+            });
+        });
+    });
+});