Merge pull request #2975 from kirillrogovoy/refactor-less-error

Refactor LessError and lesscHelper.formatError

bin/lessc

@@ -37,7 +37,7 @@ var less = require('../lib/less-node'),
         modifyVars: null,
         urlArgs: '',
         plugins: plugins
-    }; 
+    };
 
 if (less.options) {
     for (var i = 0, keys = Object.keys(options); i < keys.length; i++) {
@@ -310,7 +310,11 @@ function render() {
                 }
             },
             function(err) {
-                less.writeError(err, options);
+                if (!options.silent) {
+                    console.error(err.toString({
+                        stylize: options.color && less.lesscHelper.stylize
+                    }));
+                }
                 process.exitCode = 1;
             });
     };

lib/less-node/index.js

@@ -14,62 +14,6 @@ less.FileManager = FileManager;
 less.UrlFileManager = UrlFileManager;
 less.options = less.options || {};
 
-less.formatError = function(ctx, options) {
-    options = options || {};
-
-    var message = "";
-    var extract = ctx.extract;
-    var error = [];
-    var stylize = options.color ? lesscHelper.stylize : function (str) { return str; };
-
-    // only output a stack if it isn't a less error
-    if (ctx.stack && !ctx.type) { return stylize(ctx.stack, 'red'); }
-
-    if (!ctx.hasOwnProperty('index') || !extract) {
-        return ctx.stack || ctx.message;
-    }
-
-    if (typeof extract[0] === 'string') {
-        error.push(stylize((ctx.line - 1) + ' ' + extract[0], 'grey'));
-    }
-
-    if (typeof extract[1] === 'string') {
-        var errorTxt = ctx.line + ' ';
-        if (extract[1]) {
-            errorTxt += extract[1].slice(0, ctx.column) +
-                            stylize(stylize(stylize(extract[1].substr(ctx.column, 1), 'bold') +
-                            extract[1].slice(ctx.column + 1), 'red'), 'inverse');
-        }
-        error.push(errorTxt);
-    }
-
-    if (typeof extract[2] === 'string') {
-        error.push(stylize((ctx.line + 1) + ' ' + extract[2], 'grey'));
-    }
-    error = error.join('\n') + stylize('', 'reset') + '\n';
-
-    message += stylize(ctx.type + 'Error: ' + ctx.message, 'red');
-    if (ctx.filename) {
-        message += stylize(' in ', 'red') + ctx.filename +
-            stylize(' on line ' + ctx.line + ', column ' + (ctx.column + 1) + ':', 'grey');
-    }
-
-    message += '\n' + error;
-
-    if (ctx.callLine) {
-        message += stylize('from ', 'red') + (ctx.filename || '') + '/n';
-        message += stylize(ctx.callLine, 'grey') + ' ' + ctx.callExtract + '/n';
-    }
-
-    return message;
-};
-
-less.writeError = function (ctx, options) {
-    options = options || {};
-    if (options.silent) { return; }
-    console.error(less.formatError(ctx, options));
-};
-
 // provide image-size functionality
 require('./image-size')(less.environment);
 

lib/less/less-error.js

@@ -1,7 +1,27 @@
-var utils = require("./utils");
-
+var utils = require('./utils');
+/**
+ * This is a centralized class of any error that could be thrown internally (mostly by the parser).
+ * Besides standard .message it keeps some additional data like a path to the file where the error
+ * occurred along with line and column numbers.
+ *
+ * @class
+ * @extends Error
+ * @type {module.LessError}
+ *
+ * @prop {string} type
+ * @prop {string} filename
+ * @prop {number} index
+ * @prop {number} line
+ * @prop {number} column
+ * @prop {number} callLine
+ * @prop {number} callExtract
+ * @prop {string[]} extract
+ *
+ * @param {Object} e              - An error object to wrap around or just a descriptive object
+ * @param {Object} importManager  - An instance of ImportManager (see import-manager.js)
+ * @param {string} [currentFilename]
+ */
 var LessError = module.exports = function LessError(e, importManager, currentFilename) {
-
     Error.call(this);
 
     var filename = e.filename || currentFilename;
@@ -40,3 +60,60 @@ if (typeof Object.create === 'undefined') {
 }
 
 LessError.prototype.constructor = LessError;
+
+/**
+ * An overridden version of the default Object.prototype.toString
+ * which uses additional information to create a helpful message.
+ *
+ * @param {Object} options
+ * @returns {string}
+ */
+LessError.prototype.toString = function(options) {
+    options = options || {};
+
+    var message = '';
+    var extract = this.extract;
+    var error = [];
+    var stylize = function (str) { return str; };
+    if (options.stylize) {
+        var type = typeof options.stylize;
+        if (type !== 'function') {
+            throw Error('options.stylize should be a function, got a ' + type + '!');
+        }
+        stylize = options.stylize;
+    }
+
+    if (typeof extract[0] === 'string') {
+        error.push(stylize((this.line - 1) + ' ' + extract[0], 'grey'));
+    }
+
+    if (typeof extract[1] === 'string') {
+        var errorTxt = this.line + ' ';
+        if (extract[1]) {
+            errorTxt += extract[1].slice(0, this.column) +
+                stylize(stylize(stylize(extract[1].substr(this.column, 1), 'bold') +
+                    extract[1].slice(this.column + 1), 'red'), 'inverse');
+        }
+        error.push(errorTxt);
+    }
+
+    if (typeof extract[2] === 'string') {
+        error.push(stylize((this.line + 1) + ' ' + extract[2], 'grey'));
+    }
+    error = error.join('\n') + stylize('', 'reset') + '\n';
+
+    message += stylize(this.type + 'Error: ' + this.message, 'red');
+    if (this.filename) {
+        message += stylize(' in ', 'red') + this.filename +
+            stylize(' on line ' + this.line + ', column ' + (this.column + 1) + ':', 'grey');
+    }
+
+    message += '\n' + error;
+
+    if (this.callLine) {
+        message += stylize('from ', 'red') + (this.filename || '') + '/n';
+        message += stylize(this.callLine, 'grey') + ' ' + this.callExtract + '/n';
+    }
+
+    return message;
+};

test/index.js

@@ -18,42 +18,49 @@ function getErrorPathReplacementFunction(dir) {
 console.log("\n" + stylize("Less", 'underline') + "\n");
 
 lessTester.prepBomTest();
-lessTester.runTestSet({strictMath: true, relativeUrls: true, silent: true, javascriptEnabled: true});
-lessTester.runTestSet({strictMath: true, strictUnits: true, javascriptEnabled: true}, "errors/",
-     lessTester.testErrors, null, getErrorPathReplacementFunction("errors"));
-lessTester.runTestSet({strictMath: true, strictUnits: true, javascriptEnabled: false}, "no-js-errors/",
-    lessTester.testErrors, null, getErrorPathReplacementFunction("no-js-errors"));
-lessTester.runTestSet({strictMath: true, dumpLineNumbers: 'comments'}, "debug/", null,
-    function(name) { return name + '-comments'; });
-lessTester.runTestSet({strictMath: true, dumpLineNumbers: 'mediaquery'}, "debug/", null,
-    function(name) { return name + '-mediaquery'; });
-lessTester.runTestSet({strictMath: true, dumpLineNumbers: 'all'}, "debug/", null,
-    function(name) { return name + '-all'; });
-lessTester.runTestSet({strictMath: true, relativeUrls: false, rootpath: "folder (1)/"}, "static-urls/");
-lessTester.runTestSet({strictMath: true, compress: true}, "compression/");
-lessTester.runTestSet({strictMath: true, strictUnits: true}, "strict-units/");
-lessTester.runTestSet({}, "legacy/");
-lessTester.runTestSet({strictMath: true, strictUnits: true, sourceMap: true, globalVars: true }, "sourcemaps/",
-    lessTester.testSourcemap, null, null,
-    function(filename, type, baseFolder) {
-        if (type === "vars") {
-            return path.join(baseFolder, filename) + '.json';
-        }
-        return path.join('test/sourcemaps', filename) + '.json';
-    });
-lessTester.runTestSet({strictMath: true, strictUnits: true, sourceMap: {sourceMapFileInline: true}}, "sourcemaps-empty/", lessTester.testEmptySourcemap);
-lessTester.runTestSet({globalVars: true, banner: "/**\n  * Test\n  */\n"}, "globalVars/",
-    null, null, null, function(name, type, baseFolder) { return path.join(baseFolder, name) + '.json'; });
-lessTester.runTestSet({modifyVars: true}, "modifyVars/",
-    null, null, null, function(name, type, baseFolder) { return path.join(baseFolder, name) + '.json'; });
-lessTester.runTestSet({urlArgs: '424242'}, "url-args/");
-lessTester.runTestSet({paths: ['test/data/', 'test/less/import/']}, "include-path/");
-lessTester.runTestSet({paths: 'test/data/'}, "include-path-string/");
-lessTester.runTestSet({plugin: 'test/plugins/postprocess/'}, "postProcessorPlugin/");
-lessTester.runTestSet({plugin: 'test/plugins/preprocess/'}, "preProcessorPlugin/");
-lessTester.runTestSet({plugin: 'test/plugins/visitor/'}, "visitorPlugin/");
-lessTester.runTestSet({plugin: 'test/plugins/filemanager/'}, "filemanagerPlugin/");
-lessTester.runTestSet({}, "no-strict-math/");
+var testMap = [
+    [{strictMath: true, relativeUrls: true, silent: true, javascriptEnabled: true}],
+    [{strictMath: true, strictUnits: true, javascriptEnabled: true}, "errors/",
+        lessTester.testErrors, null, getErrorPathReplacementFunction("errors")],
+    [{strictMath: true, strictUnits: true, javascriptEnabled: false}, "no-js-errors/",
+        lessTester.testErrors, null, getErrorPathReplacementFunction("no-js-errors")],
+    [{strictMath: true, dumpLineNumbers: 'comments'}, "debug/", null,
+        function(name) { return name + '-comments'; }],
+    [{strictMath: true, dumpLineNumbers: 'mediaquery'}, "debug/", null,
+        function(name) { return name + '-mediaquery'; }],
+    [{strictMath: true, dumpLineNumbers: 'all'}, "debug/", null,
+        function(name) { return name + '-all'; }],
+    [{strictMath: true, relativeUrls: false, rootpath: "folder (1)/"}, "static-urls/"],
+    [{strictMath: true, compress: true}, "compression/"],
+    [{strictMath: true, strictUnits: true}, "strict-units/"],
+    [{}, "legacy/"],
+    [{strictMath: true, strictUnits: true, sourceMap: true, globalVars: true }, "sourcemaps/",
+        lessTester.testSourcemap, null, null,
+        function(filename, type, baseFolder) {
+            if (type === "vars") {
+                return path.join(baseFolder, filename) + '.json';
+            }
+            return path.join('test/sourcemaps', filename) + '.json';
+        }],
+    [{strictMath: true, strictUnits: true, sourceMap: {sourceMapFileInline: true}},
+        "sourcemaps-empty/", lessTester.testEmptySourcemap],
+    [{globalVars: true, banner: "/**\n  * Test\n  */\n"}, "globalVars/",
+        null, null, null, function(name, type, baseFolder) { return path.join(baseFolder, name) + '.json'; }],
+    [{modifyVars: true}, "modifyVars/",
+        null, null, null, function(name, type, baseFolder) { return path.join(baseFolder, name) + '.json'; }],
+    [{urlArgs: '424242'}, "url-args/"],
+    [{paths: ['test/data/', 'test/less/import/']}, "include-path/"],
+    [{paths: 'test/data/'}, "include-path-string/"],
+    [{plugin: 'test/plugins/postprocess/'}, "postProcessorPlugin/"],
+    [{plugin: 'test/plugins/preprocess/'}, "preProcessorPlugin/"],
+    [{plugin: 'test/plugins/visitor/'}, "visitorPlugin/"],
+    [{plugin: 'test/plugins/filemanager/'}, "filemanagerPlugin/"],
+    [{}, "no-strict-math/"]
+];
+
+testMap.forEach(function(args) {
+    lessTester.runTestSet.apply(lessTester, args)
+});
 lessTester.testSyncronous({syncImport: true}, "import");
 lessTester.testSyncronous({syncImport: true}, "css");
 lessTester.testNoOptions();

test/less-test.js

@@ -136,20 +136,20 @@ module.exports = function() {
 
     function testErrors(name, err, compiledLess, doReplacements, sourcemap, baseFolder) {
         fs.readFile(path.join(baseFolder, name) + '.txt', 'utf8', function (e, expectedErr) {
-            process.stdout.write("- " + path.join(baseFolder, name) + ": ");
+            process.stdout.write('- ' + path.join(baseFolder, name) + ": ");
             expectedErr = doReplacements(expectedErr, baseFolder);
             if (!err) {
                 if (compiledLess) {
-                    fail("No Error", 'red');
+                    fail('No Error', 'red');
                 } else {
-                    fail("No Error, No Output");
+                    fail('No Error, No Output');
                 }
             } else {
-                var errMessage = less.formatError(err);
+                var errMessage = err.toString();
                 if (errMessage === expectedErr) {
                     ok('OK');
                 } else {
-                    difference("FAIL", expectedErr, errMessage);
+                    difference('FAIL', expectedErr, errMessage);
                 }
             }
         });
@@ -254,7 +254,7 @@ module.exports = function() {
             var doubleCallCheck = false;
             queue(function() {
                 toCSS(options, path.join(baseFolder, foldername + file), function (err, result) {
-                    
+
                     if (doubleCallCheck) {
                         totalTests++;
                         fail("less is calling back twice");