From: Jakub Pawlowicz Date: Tue, 6 Dec 2016 11:14:14 +0000 (+0100) Subject: Improves warning & error messages raised during optimizations. X-Git-Url: https://git.ndcode.org/public/gitweb.cgi?a=commitdiff_plain;h=1665868d7ed3ba0f2d8c68dcd22adc357e33f07e;p=clean-css.git 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 1adab484..abb706dc 100644 --- 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 7a42daad..c4e6830f 100644 --- 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 c652c7ab..d6146478 100644 --- 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 2f97d1f0..b73e54dd 100644 --- 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 e69a08c0..72c4c82f 100644 --- 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.']); } } })