Improves warning & error messages raised during optimizations.

Why:

* We can be more specific about raised errors now.

diff --git a/History.md b/History.md
index 1adab48..abb706d 100644 (file)
--- a/History.md
+++ b/History.md
@@ -1,6 +1,7 @@
 [3.5.0-pre / 2015-xx-xx](https://github.com/jakubpawlowicz/clean-css/compare/3.4...HEAD)
 ==================
 
+* Adds more detailed error & warning messages on top of the new tokenizer.
 * Requires Node.js 4.0+ to run.
 * Replaces the old tokenizer with a new one which doesn't use any escaping.
 * Replaces the old `@import` inlining with one on top of the new tokenizer.

diff --git a/lib/optimizer/basic.js b/lib/optimizer/basic.js
index 7a42daa..c4e6830 100644 (file)
--- a/lib/optimizer/basic.js
+++ b/lib/optimizer/basic.js
@@ -338,6 +338,7 @@ function optimizeBody(properties, context) {
   var options = context.options;
   var property, name, value;
   var valueIsUrl;
+  var propertyToken;
   var _properties = wrapForOptimizing(properties);
 
   for (var i = 0, l = _properties.length; i < l; i++) {
@@ -345,6 +346,8 @@ function optimizeBody(properties, context) {
     name = property.name;
 
     if (property.value.length === 0) {
+      propertyToken = property.all[property.position];
+      context.warnings.push('Empty property \'' + name + '\' at line ' + propertyToken[1][2][0][0] + ', column ' + propertyToken[1][2][0][1] + '. Ignoring.');
       property.unused = true;
     }
 
@@ -372,6 +375,7 @@ function optimizeBody(properties, context) {
 
       if (valueIsUrl && !context.validator.isValidUrl(value)) {
         property.unused = true;
+        context.warnings.push('Broken URL \'' + value + '\' at line ' + property.value[j][2][0][0] + ', column ' + property.value[j][2][0][1] + '. Ignoring.');
         break;
       }
 

diff --git a/lib/tokenizer/tokenize.js b/lib/tokenizer/tokenize.js
index c652c7a..d614647 100644 (file)
--- a/lib/tokenizer/tokenize.js
+++ b/lib/tokenizer/tokenize.js
@@ -364,7 +364,7 @@ function intoTokens(source, externalContext, internalContext, isNested) {
   }
 
   if (seekingValue) {
-    externalContext.warnings.push('Missing \'}\' at line ' + position.line + ', column ' + position.column);
+    externalContext.warnings.push('Missing \'}\' at line ' + position.line + ', column ' + position.column + '.');
   }
 
   if (seekingValue && buffer.length > 0) {

diff --git a/test/module-test.js b/test/module-test.js
index 2f97d1f..b73e54d 100644 (file)
--- a/test/module-test.js
+++ b/test/module-test.js
@@ -131,22 +131,7 @@ vows.describe('module tests').addBatch({
     },
     'should raise one warning': function (error, minified) {
       assert.lengthOf(minified.warnings, 1);
-      assert.equal(minified.warnings[0], 'Unexpected \'}\' in \'}\'. Ignoring.');
-    }
-  },
-  'warnings on missing closing brace': {
-    'topic': function () {
-      return new CleanCSS().minify('a{display:block');
-    },
-    'should minify correctly': function (error, minified) {
-      assert.equal(minified.styles, '');
-    },
-    'should raise no errors': function (error, minified) {
-      assert.isEmpty(minified.errors);
-    },
-    'should raise one warning': function (error, minified) {
-      assert.lengthOf(minified.warnings, 1);
-      assert.equal(minified.warnings[0], 'Missing \'}\' after \'display:block\'. Ignoring.');
+      assert.equal(minified.warnings[0], 'Unexpected \'}\' at line 1, column 16.');
     }
   },
   'warnings on unexpected body': {
@@ -154,17 +139,18 @@ vows.describe('module tests').addBatch({
       return new CleanCSS().minify('a{display:block}color:#535353}p{color:red}');
     },
     'should minify correctly': function (error, minified) {
-      assert.equal(minified.styles, 'a{display:block}p{color:red}');
+      assert.equal(minified.styles, 'a{display:block}');
     },
     'should raise no errors': function (error, minified) {
       assert.isEmpty(minified.errors);
     },
     'should raise one warning': function (error, minified) {
-      assert.lengthOf(minified.warnings, 1);
-      assert.equal(minified.warnings[0], 'Unexpected content: \'color:#535353}\'. Ignoring.');
+      assert.lengthOf(minified.warnings, 2);
+      assert.equal(minified.warnings[0], 'Unexpected \'}\' at line 1, column 29.');
+      assert.equal(minified.warnings[1], 'Invalid selector \'color:#535353}p\' at line 1, column 16. Ignoring.');
     }
   },
-  'warnings on invalid properties': {
+  'warnings on empty properties': {
     'topic': function () {
       return new CleanCSS().minify('a{color:}');
     },
@@ -176,7 +162,7 @@ vows.describe('module tests').addBatch({
     },
     'should raise one warning': function (error, minified) {
       assert.lengthOf(minified.warnings, 1);
-      assert.equal(minified.warnings[0], 'Empty property \'color\' inside \'a\' selector. Ignoring.');
+      assert.equal(minified.warnings[0], 'Empty property \'color\' at line 1, column 2. Ignoring.');
     }
   },
   'warnings on broken urls': {
@@ -184,44 +170,15 @@ vows.describe('module tests').addBatch({
       return new CleanCSS().minify('a{background:url(image/}');
     },
     'should output correct content': function (error, minified) {
-      assert.equal(minified.styles, 'a{background:url(image/}');
-    },
-    'should raise no errors': function (error, minified) {
-      assert.isEmpty(minified.errors.length);
-    },
-    'should raise one warning': function (error, minified) {
-      assert.lengthOf(minified.warnings, 1);
-      assert.equal(minified.warnings[0], 'Broken URL declaration: \'url(image/\'.');
-    }
-  },
-  'warnings on broken imports': {
-    'topic': function () {
-      return new CleanCSS().minify('@impor');
-    },
-    'should output correct content': function (error, minified) {
-      assert.isEmpty(minified.styles);
-    },
-    'should raise no errors': function (error, minified) {
-      assert.isEmpty(minified.errors.length);
-    },
-    'should raise one warning': function (error, minified) {
-      assert.lengthOf(minified.warnings, 1);
-      assert.equal(minified.warnings[0], 'Broken declaration: \'@impor\'.');
-    }
-  },
-  'warnings on broken comments': {
-    'topic': function () {
-      return new CleanCSS().minify('a{}/* ');
-    },
-    'should output correct content': function (error, minified) {
-      assert.isEmpty(minified.styles);
+      assert.equal(minified.styles, '');
     },
     'should raise no errors': function (error, minified) {
       assert.isEmpty(minified.errors.length);
     },
     'should raise one warning': function (error, minified) {
-      assert.lengthOf(minified.warnings, 1);
-      assert.equal(minified.warnings[0], 'Broken comment: \'/* \'.');
+      assert.lengthOf(minified.warnings, 2);
+      assert.equal(minified.warnings[0], 'Missing \'}\' at line 1, column 24.');
+      assert.equal(minified.warnings[1], 'Broken URL \'url(image/\' at line 1, column 13. Ignoring.');
     }
   },
   'no errors': {
@@ -241,7 +198,7 @@ vows.describe('module tests').addBatch({
         minifier.minify('@import url(/some/fake/file);', function (errors) {
           assert.isArray(errors);
           assert.lengthOf(errors, 1);
-          assert.equal(errors[0], 'Broken @import declaration of "/some/fake/file"');
+          assert.equal(errors[0], 'Ignoring local @import of "/some/fake/file" as resource is missing.');
         });
       });
     }
@@ -254,10 +211,25 @@ vows.describe('module tests').addBatch({
       minifier.minify('@import url(/some/fake/file);');
       minifier.minify('@import url(/some/fake/file);', function (errors) {
         assert.lengthOf(errors, 1);
-        assert.equal(errors[0], 'Broken @import declaration of "/some/fake/file"');
+        assert.equal(errors[0], 'Ignoring local @import of "/some/fake/file" as resource is missing.');
       });
     }
   },
+  'error on broken imports': {
+    'topic': function () {
+      return new CleanCSS().minify('@import test;');
+    },
+    'should output correct content': function (error, minified) {
+      assert.isEmpty(minified.styles);
+    },
+    'should raise no warnings': function (error, minified) {
+      assert.isEmpty(minified.warnings.length);
+    },
+    'should raise one error': function (error, minified) {
+      assert.lengthOf(minified.errors, 1);
+      assert.equal(minified.errors[0], 'Ignoring local @import of "test" as resource is missing.');
+    }
+  },
   'local imports': {
     'inside a comment preceding a quote': {
       'topic': function () {
@@ -280,7 +252,7 @@ vows.describe('module tests').addBatch({
         assert.isEmpty(minified.errors);
       },
       'has a warning': function (minified) {
-        assert.deepEqual(minified.warnings, []);
+        assert.deepEqual(minified.warnings, ['Skipping remote @import of "http://jakubpawlowicz.com/styles.css" as no callback given.']);
       }
     },
     'after content': {
@@ -294,7 +266,7 @@ vows.describe('module tests').addBatch({
         assert.isEmpty(minified.errors);
       },
       'has a warning': function (minified) {
-        assert.deepEqual(minified.warnings, ['Ignoring remote @import of "http://jakubpawlowicz.com/styles.css" as no callback given.']);
+        assert.deepEqual(minified.warnings, ['Ignoring remote @import of "http://jakubpawlowicz.com/styles.css" as no callback given and after other content.']);
       }
     },
     'after local import': {
@@ -308,7 +280,7 @@ vows.describe('module tests').addBatch({
         assert.isEmpty(minified.errors);
       },
       'has a warning': function (minified) {
-        assert.deepEqual(minified.warnings, ['Ignoring remote @import of "http://jakubpawlowicz.com/styles.css" as no callback given.']);
+        assert.deepEqual(minified.warnings, ['Skipping remote @import of "http://jakubpawlowicz.com/styles.css" as no callback given.']);
       }
     },
     'after remote import': {
@@ -322,7 +294,10 @@ vows.describe('module tests').addBatch({
         assert.isEmpty(minified.errors);
       },
       'has a warning': function (minified) {
-        assert.deepEqual(minified.warnings, []);
+        assert.deepEqual(minified.warnings, [
+          'Skipping remote @import of "http://jakubpawlowicz.com/reset.css" as no callback given.',
+          'Skipping remote @import of "http://jakubpawlowicz.com/styles.css" as no callback given.'
+        ]);
       }
     }
   },

diff --git a/test/tokenizer/tokenize-test.js b/test/tokenizer/tokenize-test.js
index e69a08c..72c4c82 100644 (file)
--- a/test/tokenizer/tokenize-test.js
+++ b/test/tokenizer/tokenize-test.js
@@ -3379,7 +3379,7 @@ vows.describe(tokenize)
         return warnings;
       },
       'logs them correctly': function (warnings) {
-        assert.deepEqual(warnings, ['Missing \'}\' at line 1, column 15']);
+        assert.deepEqual(warnings, ['Missing \'}\' at line 1, column 15.']);
       }
     }
   })