Fixes #139 - adds consistent error and warning handling for CLI and library.

* Does not throw errors anymore, instead prints them out to STDERR and exits with status 1.
* Adds two new fields to CleanCSS objects - warnings and errors.
* Updates imports/inliner.js to not act as a singleton.
* Adds 'both root and output file given' warning to inliner.

diff --git a/History.md b/History.md
index 2ecc4cc..5bddb55 100644 (file)
--- a/History.md
+++ b/History.md
@@ -5,6 +5,7 @@
 * Adds simplified and much faster empty elements removal.
 * Adds missing `@import` processing to our benchmark (run via `npm run bench`).
 * Fixed issue [#138](https://github.com/GoalSmashers/clean-css/issues/138) - makes CleanCSS interface OO.
+* Fixed issue [#139](https://github.com/GoalSmashers/clean-css/issues/138) - consistent error & warning handling.
 * Fixed issue [#145](https://github.com/GoalSmashers/clean-css/issues/145) - debug mode in library too.
 * Fixed issue [#157](https://github.com/GoalSmashers/clean-css/issues/157) - gets rid of `removeEmpty` option.
 * Fixed issue [#159](https://github.com/GoalSmashers/clean-css/issues/159) - escaped quotes inside content.

diff --git a/bin/cleancss b/bin/cleancss
index c136bc4..e73ebd5 100755 (executable)
--- a/bin/cleancss
+++ b/bin/cleancss
@@ -113,6 +113,12 @@ function minify(data) {
     console.error('Time spent: %dms', minifier.stats.timeSpent);
   }
 
+  outputFeedback(minifier.errors, true);
+  outputFeedback(minifier.warnings);
+
+  if (minifier.errors.length > 0)
+    process.exit(1);
+
   return minified;
 }
 
@@ -122,3 +128,11 @@ function output(minified) {
   else
     process.stdout.write(minified);
 };
+
+function outputFeedback(messages, isError) {
+  var prefix = isError ? '\x1B[31mERROR\x1B[39m:' : 'WARNING:';
+
+  messages.forEach(function(message) {
+    console.error('%s %s', prefix, message);
+  });
+};

diff --git a/lib/clean.js b/lib/clean.js
index 74981c9..af9c7e0 100644 (file)
--- a/lib/clean.js
+++ b/lib/clean.js
@@ -25,6 +25,10 @@ var SelectorsOptimizer = require('./selectors/optimizer');
 module.exports = function(options) {
   var lineBreak = process.platform == 'win32' ? '\r\n' : '\n';
   var stats = {};
+  var context = {
+    errors: [],
+    warnings: []
+  };
 
   options = options || {};
   options.keepBreaks = options.keepBreaks || false;
@@ -71,7 +75,7 @@ module.exports = function(options) {
     var expressionsProcessor = new ExpressionsProcessor();
     var freeTextProcessor = new FreeTextProcessor();
     var urlsProcessor = new UrlsProcessor();
-    var importInliner = new ImportInliner();
+    var importInliner = new ImportInliner(context);
 
     if (options.processImport) {
       // inline all imports
@@ -265,7 +269,7 @@ module.exports = function(options) {
       data = urlsProcessor.restore(data);
     });
     replace(function rebaseUrls() {
-      data = options.noRebase ? data : UrlRebase.process(data, options);
+      data = options.noRebase ? data : new UrlRebase(options, context).process(data);
     });
     replace(function restoreFreeText() {
       data = freeTextProcessor.restore(data);
@@ -309,9 +313,11 @@ module.exports = function(options) {
   };
 
   return {
+    errors: context.errors,
     lineBreak: lineBreak,
     options: options,
     minify: minify,
-    stats: stats
+    stats: stats,
+    warnings: context.warnings
   };
 };

diff --git a/lib/images/url-rebase.js b/lib/images/url-rebase.js
index 78792dc..5b24954 100644 (file)
--- a/lib/images/url-rebase.js
+++ b/lib/images/url-rebase.js
@@ -2,8 +2,8 @@ var path = require('path');
 
 var UrlRewriter = require('./url-rewriter');
 
-module.exports = {
-  process: function(data, options) {
+module.exports = function UrlRebase(options, context) {
+  var process = function(data) {
     var rebaseOpts = {
       absolute: !!options.root,
       relative: !options.root && !!options.target,
@@ -13,6 +13,9 @@ module.exports = {
     if (!rebaseOpts.absolute && !rebaseOpts.relative)
       return data;
 
+    if (rebaseOpts.absolute && !!options.target)
+      context.warnings.push('Both \'root\' and output file given so rebasing URLs as absolute paths');
+
     if (rebaseOpts.absolute)
       rebaseOpts.toBase = path.resolve(options.root);
 
@@ -23,5 +26,9 @@ module.exports = {
       return data;
 
     return UrlRewriter.process(data, rebaseOpts);
-  }
+  };
+
+  return {
+    process: process
+  };
 };

diff --git a/lib/imports/inliner.js b/lib/imports/inliner.js
index e1e694d..f0c793a 100644 (file)
--- a/lib/imports/inliner.js
+++ b/lib/imports/inliner.js
@@ -3,7 +3,7 @@ var path = require('path');
 
 var UrlRewriter = require('../images/url-rewriter');
 
-module.exports = function Inliner() {
+module.exports = function Inliner(context) {
   var process = function(data, options) {
     var tempData = [];
     var nextStart = 0;
@@ -108,8 +108,10 @@ module.exports = function Inliner() {
 
     var fullPath = path.resolve(path.join(relativeTo, importedFile));
 
-    if (!fs.existsSync(fullPath) || !fs.statSync(fullPath).isFile())
-      throw new Error('Broken @import declaration of "' + importedFile + '"');
+    if (!fs.existsSync(fullPath) || !fs.statSync(fullPath).isFile()) {
+      context.errors.push('Broken @import declaration of "' + importedFile + '"');
+      return '';
+    }
 
     if (options.visited.indexOf(fullPath) != -1)
       return '';

diff --git a/test/binary-test.js b/test/binary-test.js
index 0e4461c..d4b4a7a 100644 (file)
--- a/test/binary-test.js
+++ b/test/binary-test.js
@@ -112,9 +112,10 @@ exports.commandsSuite = vows.describe('binary commands').addBatch({
     }
   }),
   'no relative to path': binaryContext('./test/data/partials-absolute/base.css', {
-    'should not be able to resolve it fully': function(error, stdout) {
+    'should not be able to resolve it fully': function(error, stdout, stderr) {
       assert.equal(stdout, '');
       assert.notEqual(error, null);
+      assert.notEqual(stderr, '');
     }
   }),
   'relative to path': binaryContext('-r ./test/data ./test/data/partials-absolute/base.css', {

diff --git a/test/custom-test.js b/test/custom-test.js
index eef8039..c01b8fc 100644 (file)
--- a/test/custom-test.js
+++ b/test/custom-test.js
@@ -39,5 +39,48 @@ vows.describe('clean-custom').addBatch({
     'should give efficiency': function(minifier) {
       assert.equal(minifier.stats.efficiency, 0.25);
     }
-  }
+  },
+  'no warnings': {
+    topic: function() {
+      var minifier = new CleanCSS();
+      minifier.minify('a{color:red}');
+      return minifier;
+    },
+    'if no reasons given': function(minifier) {
+      assert.deepEqual(minifier.warnings, []);
+    }
+  },
+  'warnings': {
+    topic: function() {
+      var minifier = new CleanCSS({ root: 'test/data', target: 'custom-warnings.css' });
+      minifier.minify('a{color:red}');
+      return minifier;
+    },
+    'if both root and output used reasons given': function(minifier) {
+      assert.equal(minifier.warnings.length, 1);
+      assert.match(minifier.warnings[0], /Both 'root' and output file given/);
+    }
+  },
+  'no errors': {
+    topic: function() {
+      var minifier = new CleanCSS();
+      minifier.minify('a{color:red}');
+      return minifier;
+    },
+    'if no reasons given': function(minifier) {
+      assert.deepEqual(minifier.errors, []);
+    }
+  },
+  'errors': {
+    topic: function() {
+      return new CleanCSS();
+    },
+    'if both root and output used reasons given': function(minifier) {
+      assert.doesNotThrow(function() {
+        minifier.minify('@import url(/some/fake/file);');
+      });
+      assert.equal(minifier.errors.length, 1);
+      assert.equal(minifier.errors[0], 'Broken @import declaration of "/some/fake/file"');
+    }
+  },
 }).export(module);

diff --git a/test/unit-test.js b/test/unit-test.js
index e15aecb..f74652d 100644 (file)
--- a/test/unit-test.js
+++ b/test/unit-test.js
@@ -9,16 +9,10 @@ var ColorShortener = require('../lib/colors/shortener');
 var lineBreak = process.platform == 'win32' ? '\r\n' : '\n';
 var cssContext = function(groups, options) {
   var context = {};
-  var clean = function(expectedCSS) {
+  var clean = function(expectedCss) {
     return function(css) {
-      var minifiedCSS = null;
-      try {
-        minifiedCSS = new CleanCSS(options).minify(css);
-      } catch (e) {
-        // swallow - minifiedCSS is set to null and that's the new expected value
-      }
-
-      assert.equal(minifiedCSS, expectedCSS);
+      var minifiedCss = new CleanCSS(options).minify(css);
+      assert.equal(minifiedCss, expectedCss);
     };
   };
 
@@ -926,11 +920,11 @@ title']{display:block}",
   '@import': cssContext({
     'empty': [
       "@import url();",
-      null
+      ''
     ],
     'of an unknown file': [
       "@import url('fake.css');",
-      null
+      ''
     ],
     'of a http file': "@import url(http://pro.goalsmashers.com/test.css);",
     'of a https file': [
@@ -945,7 +939,7 @@ title']{display:block}",
     'of a remote file via // url with media': "@import url(//pro.goalsmashers.com/test.css) screen,tv;",
     'of a directory': [
       "@import url(test/data/partials);",
-      null
+      ''
     ],
     'of a real file': [
       "@import url(test/data/partials/one.css);",
@@ -1043,7 +1037,7 @@ title']{display:block}",
   '@import with absolute paths': cssContext({
     'of an unknown file': [
       "@import url(/fake.css);",
-      null
+      ''
     ],
     'of a real file': [
       "@import url(/partials/one.css);",