Repository: lesshint/lesshint
Pull request #365 (2017-03-16 20:08:58)

Linter refactor
<!--
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!
-->

The `linter.js` file has grown out of hand so I decided to refactor it into a proper class, renamed some methods, merged others, but mostly laid the ground work for a better structure for inline configuration comments.

**Which issue, if any, does this resolve?**
N/A

**Is there anything in this PR that needs extra explaining or should something specific be focused on?**
The test changes could use an extra pair of eyes, but I think they're alright since this isn't a public API. The rest of the changes are basically copy/paste and some renaming.

~~Still a WIP so don't merge it, but feedback is appreciated!~~

Edit: No longer a WIP, ready for review!

jwilsson: I'm gonna merge this today or in the next few days, get your feedback in!
shellscape: LGTM!

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index fadf787..fbdd22e 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,4 +1,15 @@
 # Changelog
+## 3.1.1 (2017-03-27)
+* Fixed an issue where `finalNewline` would report the wrong position when double `\n` characters were used. ([d234a20](https://github.com/lesshint/lesshint/commit/d234a20b91184e842685d87c646eed122e24bbe5))
+* Fixed an issue where `spaceAroundOperator` would erroneously report numbers surrounded by parentheses. ([98953c0](https://github.com/lesshint/lesshint/commit/98953c0f4a0b1d6c2e4bf03ff3c8a92701e27ae7))
+* Made sure internal error classes can handle both strings and error objects. ([2a8a52a](https://github.com/lesshint/lesshint/commit/2a8a52a150e0615ea3d6f46a12a6389ea09890ab))
+* Fixed an issue where `urlQuotes` would erroneously report variables. ([4508e81](https://github.com/lesshint/lesshint/commit/4508e81a6cdd5ddc6b380ee58fea64be57fff6b9))
+* Updated `gulp-mocha` to `4.x`. ([7f392ee](https://github.com/lesshint/lesshint/commit/7f392eea547692e80e8e72f0fff4bfb5d4028446))
+* Replaced `istanbul` with `nyc`. ([92d59f8](https://github.com/lesshint/lesshint/commit/92d59f832904012dbc73b213c584edf9faf56a3a))
+* Updated `sinon` to `2.x`. ([f94cae6](https://github.com/lesshint/lesshint/commit/f94cae67eb3b61993335cc216cd3a8525af57b18))
+* Fixed an `singleLinePerProperty` example config typo. ([5acc9d7](https://github.com/lesshint/lesshint/commit/5acc9d709abdc1f92624d8158d0e963fcd47638c))
+* Added a more prominent way to access rule docs from the `README`. ([bc6cc3e](https://github.com/lesshint/lesshint/commit/bc6cc3e06c30e4e84569845f87d591f1a9a8f84f))
+
 ## 3.1.0 (2017-02-16)
 * Added a `variableValue` linter. ([1a9d48d](https://github.com/lesshint/lesshint/commit/1a9d48d09e8d5eada852b428330eea40214f7c73))
 * Added a `universalSelector` linter. ([781e372](https://github.com/lesshint/lesshint/commit/781e372b48523def310fac7b32de5675952a8b0b))
diff --git a/lib/lesshint.js b/lib/lesshint.js
index 59b676f..367515a 100644
--- a/lib/lesshint.js
+++ b/lib/lesshint.js
@@ -4,7 +4,7 @@ const LesshintError = require('./errors/lesshint-error');
 const configLoader = require('./config-loader');
 const minimatch = require('minimatch');
 const merge = require('lodash.merge');
-const linter = require('./linter');
+const Linter = require('./linter');
 const path = require('path');
 const fs = require('fs');
 
@@ -102,9 +102,12 @@ class Lesshint {
     checkString (input, checkPath) {
         let result;
 
+        checkPath = checkPath || '';
+
         try {
-            checkPath = checkPath || '';
-            result = linter.lint(input, checkPath, this.config);
+            const linter = new Linter(input, checkPath, this.config);
+
+            result = linter.lint();
         } catch (e) {
             if (e.name !== 'CssSyntaxError') {
                 /**
@@ -122,7 +125,7 @@ class Lesshint {
                 line: e.line,
                 linter: 'parse error',
                 message: e.reason,
-                severity: linter.resultSeverity.error
+                severity: 'error'
             }];
         }
 
diff --git a/lib/linter.js b/lib/linter.js
index a50a764..c789e05 100644
--- a/lib/linter.js
+++ b/lib/linter.js
@@ -47,62 +47,41 @@ const defaultLinters = [
     require('./linters/zero_unit')
 ];
 
-const parseRules = function (node) {
-    let rules = node.text.match(/^\s*lesshint\s+(.*)/);
+/**
+  * Tests selectors for pseudo classes/selectors. eg. :not(), :active
+  * Temporary fix for https://github.com/webschik/postcss-less/issues/56
+  */
+const rPseudo = /::?[^ ,:.]+/g;
 
-    if (!rules) {
-        return {};
-    }
+class Linter {
+    constructor (source, fullPath, options) {
+        // Include trailing line endings, needs to run before any normalization
+        this.linesRaw = source.match(/([^\r\n]*(?:\r?\n)|[^\r\n]+)/g);
 
-    rules = rules[1].trim();
-    rules = rules.replace(/([a-z0-9]+)\s*:/ig, '"$1":');
+        source = source.replace(/\r\n|\r|\n/g, '\n'); // Normalize line endings. See #40
 
-    try {
-        rules = JSON.parse('{' + rules + '}');
-    } catch (e) {
-        throw new Error('Invalid inline option on line ' + node.source.start.line);
-    }
+        const tree = this.parse(source);
 
-    // If it's only shorthand enabled or disabled, account for that
-    Object.keys(rules).forEach(function (rule) {
-        if (typeof rules[rule] === 'boolean') {
-            rules[rule] = {
-                enabled: rules[rule]
-            };
-        }
-    });
+        this.ast = tree.root.toResult();
+        this.ast = Object.freeze(this.ast);
 
-    return rules;
-};
+        this.filename = path.basename(fullPath);
+        this.fullPath = path.resolve(process.cwd(), fullPath);
+        this.lines = source.split('\n');
+        this.options = options;
+        this.results = [];
 
-const getInlineOptions = function (node) {
-    node = node || {};
+        this.severity = {
+            error: 'error',
+            warning: 'warning'
+        };
 
-    if (node.type !== 'comment') {
-        return {};
+        this.configure();
     }
 
-    const start = node.source.start;
-
-    // Not the first thing in the file, bail
-    if (start.line !== 1 && start.column !== 1) {
-        return {};
-    }
-
-    return parseRules(node);
-};
-
-const runChecks = function (ast, fullPath, lines, linesRaw, config) {
-    const filename = path.basename(fullPath);
-    const lineOptions = [];
-    const results = [];
-
-    // Tests selectors for pseudo classes/selectors. eg. :not(), :active
-    const rPseudo = /::?[^ ,:.]+/g;
-
-    const checkNode = function (node) {
-        linters.forEach(function (linter) {
-            const options = config[linter.name];
+    checkNode (node) {
+        this.linters.forEach((linter) => {
+            const options = this.options[linter.name];
 
             // Bail if the linter isn't wanted
             if (!options || (options && options !== true && !options.enabled)) {
@@ -133,12 +112,13 @@ const runChecks = function (ast, fullPath, lines, linesRaw, config) {
 
             if (lint) {
                 if (!Array.isArray(lint)) {
-                    throw new Error('Linter ' + linter.name + ' must return an array.');
+                    throw new Error(`Linter "${ linter.name }" must return an array.`);
                 }
 
-                lint.forEach(function (piece) {
+                lint.forEach((piece) => {
+                    const column = piece.column || node.source.start.column;
                     const line = piece.line || node.source.start.line;
-                    const shouldIgnore = lineOptions.some(function (lineOption) {
+                    const shouldIgnore = this.lineOptions.some((lineOption) => {
                         lineOption = lineOption[linter.name];
 
                         if (!lineOption) {
@@ -152,120 +132,146 @@ const runChecks = function (ast, fullPath, lines, linesRaw, config) {
                         return;
                     }
 
-                    // Each piece of lint (linter result) can override
-                    // the default properties of the result we're adding
-                    // to the collection
+                    /**
+                     * Each piece of lint (linter result) can override
+                     * the default properties of the result we're adding
+                     * to the collection
+                     */
                     piece = merge({
-                        column: node.source.start.column,
-                        file: filename,
-                        fullPath: fullPath,
+                        column: column,
+                        file: this.filename,
+                        fullPath: this.fullPath,
                         line: line,
                         linter: linter.name,
                         message: 'Unknown linting error. Source: ' + linter.name,
-                        severity: options.severity || exports.resultSeverity.warning,
-                        source: line ? lines[line - 1] : ''
+                        position: column + this.linesRaw.slice(0, line - 1).join('').length - 1,
+                        severity: options.severity || this.severity.warning,
+                        source: line ? this.lines[line - 1] : ''
                     }, piece);
 
-                    piece.position = piece.column
-                        + linesRaw.slice(0, piece.line - 1).join('').length
-                        - 1;
-
-                    results.push(piece);
+                    this.results.push(piece);
                 });
             }
         });
-    };
+    }
 
-    // Freeze the AST so linters won't accidentally change it
-    Object.freeze(ast);
+    configure () {
+        this.linters = this.setupLinters(defaultLinters);
+        this.lineOptions = [];
 
-    // Fetch inline options and merge it with whatever passed
-    const inlineOptions = getInlineOptions(ast.root.first);
+        // Fetch all single line options
+        this.ast.root.walkComments((node) => {
+            const option = this.parseOption(node);
+            const key = Object.keys(option)[0];
 
-    config = merge(config, inlineOptions);
+            option[key] = merge(option[key], {
+                line: node.source.start.line
+            });
 
-    // Load any additional linters
-    const linters = mergeLintersFromConfig(defaultLinters, config);
+            this.lineOptions.push(option);
+        });
 
-    // Fetch all single line options before we start the actual linting
-    ast.root.walkComments(function (node) {
-        const option = parseRules(node);
-        const key = Object.keys(option)[0];
+        const inlineOptions = this.getInlineOptions(this.ast.root.first);
 
-        option[key] = merge(option[key], {
-            line: node.source.start.line
-        });
+        this.options = merge(this.options, inlineOptions);
+    }
 
-        lineOptions.push(option);
-    });
+    getInlineOptions (node) {
+        node = node || {};
 
-    // Check top-level root node
-    checkNode(ast.root);
+        if (node.type !== 'comment') {
+            return {};
+        }
 
-    // Walk through and check each node within (but not including) root
-    ast.root.walk(function (node) {
-        checkNode(node);
-    });
+        const start = node.source.start;
 
-    return results;
-};
+        // Not the first thing in the file, bail
+        if (start.line !== 1 && start.column !== 1) {
+            return {};
+        }
 
-const mergeLintersFromConfig = function (defaultLinters, config) {
-    if (!config.linters) {
-        return defaultLinters;
+        return this.parseOption(node);
     }
 
-    if (!(config.linters instanceof Array)) {
-        throw new Error('linters should be an array of file paths and/or linters');
-    }
+    lint () {
+        try {
+            this.checkNode(this.ast.root);
+
+            this.ast.root.walk((node) => {
+                this.checkNode(node);
+            });
+        } catch (e) {
+            // Just rethrow any exceptions and let someone else handle it
+            throw e;
+        }
 
-    return defaultLinters.concat(config.linters.map(requireConfigLinter));
-};
+        this.results = sortBy(this.results, 'column');
+        this.results = sortBy(this.results, 'line');
 
-const requireConfigLinter = function (linter) {
-    if (typeof linter !== 'string') {
-        return linter;
+        return this.results;
     }
 
-    let pathToLinter;
-    try {
-        pathToLinter = require.resolve(path.join(process.cwd(), linter));
-    } catch (e) {
-        pathToLinter = require.resolve(linter);
+    parse (source) {
+        return postcss([]).process(source, {
+            syntax: postcssLess
+        });
+    }
+
+    parseOption (node) {
+        let rules = node.text.match(/^\s*lesshint\s+(.*)/);
+
+        if (!rules) {
+            return {};
+        }
+
+        rules = rules[1].trim();
+        rules = rules.replace(/([a-z0-9]+)\s*:/ig, '"$1":');
+
+        try {
+            rules = JSON.parse('{' + rules + '}');
+        } catch (e) {
+            throw new Error('Invalid inline option on line ' + node.source.start.line);
+        }
+
+        // If it's only shorthand enabled or disabled, account for that
+        Object.keys(rules).forEach(function (rule) {
+            if (typeof rules[rule] === 'boolean') {
+                rules[rule] = {
+                    enabled: rules[rule]
+                };
+            }
+        });
+
+        return rules;
     }
 
-    return require(pathToLinter);
-};
+    setupLinters (defaults) {
+        if (!this.options.linters) {
+            return defaults;
+        }
 
-exports.resultSeverity = {
-    error: 'error',
-    warning: 'warning'
-};
+        if (!Array.isArray(this.options.linters)) {
+            throw new Error('Linters should be an array of file paths and/or linters');
+        }
 
-exports.lint = function (input, fullPath, config) {
-    const linesRaw = input.match(/([^\r\n]*(?:\r?\n)|[^\r\n]+)/g); // Include trailing line endings
+        const linters = this.options.linters.map((linter) => {
+            if (typeof linter !== 'string') {
+                return linter;
+            }
 
-    input = input.replace(/\r\n|\r|\n/g, '\n'); // Normalize line endings. See #40
-    fullPath = path.resolve(process.cwd(), fullPath);
+            let linterPath;
 
-    try {
-        const lines = input.split('\n');
-        const parser = this.getParser(input);
-        const ast = parser.root.toResult();
-        let results = runChecks(ast, fullPath, lines, linesRaw, config);
+            try {
+                linterPath = require.resolve(path.join(process.cwd(), linter));
+            } catch (e) {
+                linterPath = require.resolve(linter);
+            }
 
-        results = sortBy(results, 'column');
-        results = sortBy(results, 'line');
+            return require(linterPath);
+        });
 
-        return results;
-    } catch (e) {
-        // Just rethrow the exception and let someone else handle it
-        throw e;
+        return defaults.concat(linters);
     }
-};
+}
 
-exports.getParser = function (input) {
-    return postcss([]).process(input, {
-        syntax: postcssLess
-    });
-};
+module.exports = Linter;
diff --git a/package.json b/package.json
index 6caa1ab..fecb44b 100644
--- a/package.json
+++ b/package.json
@@ -1,7 +1,7 @@
 {
   "name": "lesshint",
   "description": "A tool to aid you in writing clean and consistent Less.",
-  "version": "3.1.0",
+  "version": "3.1.1",
   "main": "./lib/index.js",
   "author": {
     "name": "Jonathan Wilsson",
@@ -58,7 +58,7 @@
     "branches": 95,
     "check-coverage": true,
     "exclude": [
-        "gulpfile.js"
+      "gulpfile.js"
     ],
     "reporter": [
       "lcov",


Test patch:
diff --git a/test/plugins/noArrayLinter.js b/test/plugins/noArrayLinter.js
new file mode 100644
index 0000000..7b69e24
--- /dev/null
+++ b/test/plugins/noArrayLinter.js
@@ -0,0 +1,11 @@
+'use strict';
+
+module.exports = {
+    name: 'noArray',
+    nodeTypes: ['comment'],
+    message: 'Not an array.',
+
+    lint: function noArrayLinter () {
+        return this.message;
+    }
+};
diff --git a/test/specs/linter.js b/test/specs/linter.js
index b5c180a..ff3df32 100644
--- a/test/specs/linter.js
+++ b/test/specs/linter.js
@@ -1,12 +1,11 @@
 'use strict';
 
+const Linter = require('../../lib/linter');
 const expect = require('chai').expect;
 const path = require('path');
 const fs = require('fs');
 
 describe('linter', function () {
-    const linter = require('../../lib/linter');
-
     const readFileSync = function (filePath) {
         return fs.readFileSync(path.resolve(process.cwd(), filePath), 'utf8')
             .replace(/\r\n|\r|\n/g, '\n'); // Normalize line endings
@@ -27,9 +26,10 @@ describe('linter', function () {
                 }
             };
 
-            const results = linter.lint(source, testPath, config);
+            const linter = new Linter(source, testPath, config);
+            const result = linter.lint();
 
-            expect(results).to.have.length(2);
+            expect(result).to.have.length(2);
         });
 
         it('should handle all sorts of line endings (#40)', function () {
@@ -47,14 +47,15 @@ describe('linter', function () {
                 source: ' margin-right:10px; }'
             }];
 
-            const options = {
+            const config = {
                 spaceAfterPropertyColon: {
                     enabled: true,
                     style: 'one_space'
                 }
             };
 
-            const result = linter.lint(source, testPath, options);
+            const linter = new Linter(source, testPath, config);
+            const result = linter.lint();
 
             expect(result).to.deep.equal(expected);
         });
@@ -81,7 +82,8 @@ describe('linter', function () {
                 }
             };
 
-            const result = linter.lint(source, testPath, config);
+            const linter = new Linter(source, testPath, config);
+            const result = linter.lint();
 
             expect(result).to.deep.equal(expected);
         });
@@ -153,7 +155,8 @@ describe('linter', function () {
                 }
             };
 
-            const result = linter.lint(source, testPath, config);
+            const linter = new Linter(source, testPath, config);
+            const result = linter.lint();
 
             expect(result).to.deep.equal(expected);
         });
@@ -167,7 +170,8 @@ describe('linter', function () {
                 }
             };
 
-            const result = linter.lint(source, testPath, config);
+            const linter = new Linter(source, testPath, config);
+            const result = linter.lint();
 
             expect(result).to.have.length(0);
         });
@@ -179,7 +183,8 @@ describe('linter', function () {
                 spaceBeforeBrace: false
             };
 
-            const result = linter.lint(source, testPath, config);
+            const linter = new Linter(source, testPath, config);
+            const result = linter.lint();
 
             expect(result).to.have.length(0);
         });
@@ -191,7 +196,8 @@ describe('linter', function () {
                 stringQuotes: true
             };
 
-            const result = linter.lint(source, testPath, config);
+            const linter = new Linter(source, testPath, config);
+            const result = linter.lint();
 
             // String quotes should never be called since there no strings in the input
             expect(result).to.have.length(0);
@@ -220,7 +226,8 @@ describe('linter', function () {
                 }
             };
 
-            const result = linter.lint(source, testPath, config);
+            const linter = new Linter(source, testPath, config);
+            const result = linter.lint();
 
             expect(result).to.deep.equal(expected);
         });
@@ -252,7 +259,8 @@ describe('linter', function () {
                 }
             };
 
-            const result = linter.lint(source, testPath, config);
+            const linter = new Linter(source, testPath, config);
+            const result = linter.lint();
 
             expect(result).to.deep.equal(expected);
         });
@@ -284,7 +292,8 @@ describe('linter', function () {
                 }
             };
 
-            const result = linter.lint(source, testPath, config);
+            const linter = new Linter(source, testPath, config);
+            const result = linter.lint();
 
             expect(result).to.deep.equal(expected);
         });
@@ -323,7 +332,8 @@ describe('linter', function () {
                 }
             };
 
-            const result = linter.lint(source, testPath, config);
+            const linter = new Linter(source, testPath, config);
+            const result = linter.lint();
 
             expect(result).to.deep.equal(expected);
         });
@@ -350,7 +360,8 @@ describe('linter', function () {
                 }
             };
 
-            const result = linter.lint(source, testPath, config);
+            const linter = new Linter(source, testPath, config);
+            const result = linter.lint();
 
             expect(result).to.deep.equal(expected);
         });
@@ -377,7 +388,8 @@ describe('linter', function () {
                 }
             };
 
-            const result = linter.lint(source, testPath, config);
+            const linter = new Linter(source, testPath, config);
+            const result = linter.lint();
 
             expect(result).to.deep.equal(expected);
         });
@@ -386,18 +398,36 @@ describe('linter', function () {
             const source = readFileSync('./test/data/inline-options/options-invalid.less');
 
             expect(function () {
-                linter.lint(source, 'options-invalid.less', {});
+                const linter = new Linter(source, 'options-invalid.less', {});
+
+                linter.lint();
             }).to.throw('Invalid inline option on line 1');
         });
 
         it('should not report comma spaces for selectors that have pseudos', function () {
             const source = '.foo,\n.bar:not(.foo){}';
             const testPath = 'test.less';
+
+            const linter = new Linter(source, testPath, {});
             const result = linter.lint(source, testPath, {});
 
             expect(result).to.have.length(0);
         });
 
+        it('should throw if a linter does not return an array', function () {
+            const source = '// foo\n';
+            const config = {
+                linters: ['../test/plugins/noArrayLinter'],
+                noArray: true
+            };
+
+            expect(function () {
+                const linter = new Linter(source, 'test.less', config);
+
+                linter.lint();
+            }).to.throw('Linter "noArray" must return an array.');
+        });
+
         it('should load a custom linter (as a require path)', function () {
             const source = '// boo!\n';
             const testPath = path.resolve(process.cwd(), 'test.less');
@@ -418,7 +448,8 @@ describe('linter', function () {
                 source: source.trim()
             }];
 
-            const result = linter.lint(source, testPath, config);
+            const linter = new Linter(source, testPath, config);
+            const result = linter.lint();
 
             expect(result).to.deep.equal(expected);
         });
@@ -443,18 +474,22 @@ describe('linter', function () {
                 source: source.trim()
             }];
 
-            const result = linter.lint(source, testPath, config);
+            const linter = new Linter(source, testPath, config);
+            const result = linter.lint();
 
             expect(result).to.deep.equal(expected);
         });
     });
 
-    describe('getParser', function () {
-        it('should return a parser promise', function () {
-            const source = '.foo { color: red; }';
-            const parser = linter.getParser(source);
+    describe('configure', function () {
+        it('should throw on invalid "linters" value', function () {
+            const config = {
+                linters: 'invalid'
+            };
 
-            expect(parser).to.have.property('then'); // If the returned object has a 'then' method, we'll consider it a success
+            expect(function () {
+                new Linter('', '', config);
+            }).to.throw('Linters should be an array of file paths and/or linters');
         });
     });
 });
diff --git a/test/specs/util.js b/test/specs/util.js
index 79d722b..83ca691 100644
--- a/test/specs/util.js
+++ b/test/specs/util.js
@@ -1,6 +1,6 @@
 'use strict';
 
-const getParser = require('../../lib/linter').getParser;
+const Linter = require('../../lib/linter');
 const path = require('path');
 
 module.exports = {
@@ -14,10 +14,11 @@ module.exports = {
         delete require.cache[__filename];
 
         const filename = path.join('../../lib/linters/', path.basename(module.parent.filename));
-        let linter;
+        const linter = new Linter('', '', {});
+        let rule;
 
         try {
-            linter = require(filename);
+            rule = require(filename);
         } catch (e) {
             return {};
         }
@@ -26,10 +27,10 @@ module.exports = {
         const rPseudo = /::?[^ ,:.]+/g;
 
         return {
-            linter: linter,
-            parser: getParser,
+            linter: rule,
+            parser: linter.parse,
             parse: function (source, callback) {
-                return getParser(source).then(function (ast) {
+                return linter.parse(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