From f77d0362e74a5fe90153e903b1f8f256b927fe9c Mon Sep 17 00:00:00 2001 From: Adaline Valentina Simonian Date: Wed, 25 Aug 2021 13:38:58 -0700 Subject: [PATCH 1/7] Allow choosing EOL and appending final newline MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses #951 Adds the following options to js2svg and CLI: - eol — can be set to `lf` or `crlf`. If unspecified, js2svg uses the platform EOL. - finalNewline — defaults to false. If true, js2svg ensures any SVG output has a final newline. Tests added to cover both options. --- lib/svgo.test.js | 130 +++++++++++++++++++++++++++++++++++++++++++++ lib/svgo/coa.js | 27 ++++++++++ lib/svgo/js2svg.js | 40 ++++++++++---- 3 files changed, 188 insertions(+), 9 deletions(-) diff --git a/lib/svgo.test.js b/lib/svgo.test.js index 405810956..e073bb5c5 100644 --- a/lib/svgo.test.js +++ b/lib/svgo.test.js @@ -1,5 +1,9 @@ 'use strict'; +jest.mock('os', () => ({ + EOL: '\r\n', +})); + const { optimize } = require('./svgo.js'); test('allow to setup default preset', () => { @@ -64,6 +68,132 @@ test('allow to disable and customize plugins in preset', () => { `); }); +describe('allow to configure EOL', () => { + // See mock at top of test file + afterAll(() => { + jest.unmock('os'); + }); + + test('should default to platform EOL', () => { + const svg = ` + + + + Not standard description + + + + `; + // using toEqual because line endings matter in these tests + expect( + optimize(svg, { + js2svg: { pretty: true, indent: 2 }, + }).data + ).toEqual( + `\r\n \r\n\r\n` + ); + }); + + test('should respect EOL set to LF', () => { + const svg = ` + + + + Not standard description + + + + `; + // using toEqual because line endings matter in these tests + expect( + optimize(svg, { + js2svg: { eol: 'lf', pretty: true, indent: 2 }, + }).data + ).toEqual( + `\n \n\n` + ); + }); + + test('should respect EOL set to CRLF', () => { + const svg = ` + + + + Not standard description + + + + `; + // using toEqual because line endings matter in these tests + expect( + optimize(svg, { + js2svg: { eol: 'crlf', pretty: true, indent: 2 }, + }).data + ).toEqual( + `\r\n \r\n\r\n` + ); + }); +}); + +describe('allow to configure final newline', () => { + test('should not add final newline when unset', () => { + const svg = ` + + + + Not standard description + + + + `; + expect(optimize(svg).data).toMatchInlineSnapshot( + `""` + ); + }); + + test('should add final newline when set', () => { + const svg = ` + + + + Not standard description + + + + `; + expect( + optimize(svg, { + js2svg: { finalNewline: true }, + }).data + ).toMatchInlineSnapshot(` + " + " + `); + }); + + test('should not add extra newlines when using pretty: true', () => { + const svg = ` + + + + Not standard description + + + + `; + expect( + optimize(svg, { + js2svg: { finalNewline: true, pretty: true, indent: 2 }, + }).data + ).toMatchInlineSnapshot(` + " + + + " + `); + }); +}); + test('allow to customize precision for preset', () => { const svg = ` diff --git a/lib/svgo/coa.js b/lib/svgo/coa.js index 0248e36fb..c772a2ad4 100644 --- a/lib/svgo/coa.js +++ b/lib/svgo/coa.js @@ -54,6 +54,11 @@ module.exports = function makeProgram(program) { ) .option('--pretty', 'Make SVG pretty printed') .option('--indent ', 'Indent number when pretty printing SVGs') + .option( + '--eol ', + 'Line break to use when outputting SVG: lf, crlf. If unspecified, uses platform default.' + ) + .option('--final-newline', 'Ensure SVG ends with a line break') .option( '-r, --recursive', "Use with '--folder'. Optimizes *.svg files in folders recursively." @@ -112,6 +117,16 @@ async function action(args, opts, command) { } } + if (opts.eol != null) { + const eol = opts.eol.toLowerCase(); + if (eol !== 'lf' && eol !== 'crlf') { + console.error( + "error: option '--eol' must have one of the following values: 'lf' or 'crlf'" + ); + process.exit(1); + } + } + // --show-plugins if (opts.showPlugins) { showAvailablePlugins(); @@ -185,6 +200,18 @@ async function action(args, opts, command) { } } + // --eol + if (opts.eol) { + config.js2svg = config.js2svg || {}; + config.js2svg.eol = opts.eol; + } + + // --final-newline + if (opts.finalNewline) { + config.js2svg = config.js2svg || {}; + config.js2svg.finalNewline = true; + } + // --output if (output) { if (input.length && input[0] != '-') { diff --git a/lib/svgo/js2svg.js b/lib/svgo/js2svg.js index 69cf52bcf..5416c2b39 100644 --- a/lib/svgo/js2svg.js +++ b/lib/svgo/js2svg.js @@ -1,6 +1,6 @@ 'use strict'; -var EOL = require('os').EOL, +var platformEOL = require('os').EOL, textElems = require('../../plugins/_collections.js').textElems; var defaults = { @@ -28,6 +28,8 @@ var defaults = { encodeEntity: encodeEntity, pretty: false, useShortTags: true, + eol: undefined, + finalNewline: false, }; var entities = { @@ -64,15 +66,26 @@ function JS2SVG(config) { this.config.indent = ' '; } + this.eol = platformEOL; + + if (typeof this.config.eol === 'string' && this.config.eol) { + const configuredEOL = this.config.eol.toLowerCase(); + if (configuredEOL === 'lf') { + this.eol = '\n'; + } else if (configuredEOL === 'crlf') { + this.eol = '\r\n'; + } + } + if (this.config.pretty) { - this.config.doctypeEnd += EOL; - this.config.procInstEnd += EOL; - this.config.commentEnd += EOL; - this.config.cdataEnd += EOL; - this.config.tagShortEnd += EOL; - this.config.tagOpenEnd += EOL; - this.config.tagCloseEnd += EOL; - this.config.textEnd += EOL; + this.config.doctypeEnd += this.eol; + this.config.procInstEnd += this.eol; + this.config.commentEnd += this.eol; + this.config.cdataEnd += this.eol; + this.config.tagShortEnd += this.eol; + this.config.tagOpenEnd += this.eol; + this.config.tagCloseEnd += this.eol; + this.config.textEnd += this.eol; } this.indentLevel = 0; @@ -118,6 +131,15 @@ JS2SVG.prototype.convert = function (data) { this.indentLevel--; + if ( + this.config.finalNewline && + this.indentLevel === 0 && + svg.length > 0 && + svg[svg.length - 1] !== '\n' + ) { + svg += this.eol; + } + return { data: svg, info: { From af3b36b5e778feddb1ccf75607ac09031f5b5e44 Mon Sep 17 00:00:00 2001 From: Adaline Valentina Simonian Date: Sat, 11 Sep 2021 09:13:34 -0700 Subject: [PATCH 2/7] Move platformEOL into js2svg config defaults --- lib/svgo/js2svg.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/svgo/js2svg.js b/lib/svgo/js2svg.js index 5416c2b39..181957b2f 100644 --- a/lib/svgo/js2svg.js +++ b/lib/svgo/js2svg.js @@ -28,7 +28,7 @@ var defaults = { encodeEntity: encodeEntity, pretty: false, useShortTags: true, - eol: undefined, + eol: platformEOL, finalNewline: false, }; @@ -66,14 +66,14 @@ function JS2SVG(config) { this.config.indent = ' '; } - this.eol = platformEOL; - if (typeof this.config.eol === 'string' && this.config.eol) { const configuredEOL = this.config.eol.toLowerCase(); if (configuredEOL === 'lf') { this.eol = '\n'; } else if (configuredEOL === 'crlf') { this.eol = '\r\n'; + } else { + this.eol = this.config.eol; } } From 19ca101dff9243899bf8d2fd538d4d8c066d512e Mon Sep 17 00:00:00 2001 From: Adaline Valentina Simonian Date: Sat, 11 Sep 2021 09:16:13 -0700 Subject: [PATCH 3/7] Remove js2svg test os mock, add linebreak test --- lib/svgo.test.js | 25 ++++++++----------------- 1 file changed, 8 insertions(+), 17 deletions(-) diff --git a/lib/svgo.test.js b/lib/svgo.test.js index e073bb5c5..3e23a8ac7 100644 --- a/lib/svgo.test.js +++ b/lib/svgo.test.js @@ -1,9 +1,5 @@ 'use strict'; -jest.mock('os', () => ({ - EOL: '\r\n', -})); - const { optimize } = require('./svgo.js'); test('allow to setup default preset', () => { @@ -69,12 +65,7 @@ test('allow to disable and customize plugins in preset', () => { }); describe('allow to configure EOL', () => { - // See mock at top of test file - afterAll(() => { - jest.unmock('os'); - }); - - test('should default to platform EOL', () => { + test('should respect EOL set to LF', () => { const svg = ` @@ -87,14 +78,14 @@ describe('allow to configure EOL', () => { // using toEqual because line endings matter in these tests expect( optimize(svg, { - js2svg: { pretty: true, indent: 2 }, + js2svg: { eol: 'lf', pretty: true, indent: 2 }, }).data ).toEqual( - `\r\n \r\n\r\n` + `\n \n\n` ); }); - test('should respect EOL set to LF', () => { + test('should respect EOL set to CRLF', () => { const svg = ` @@ -107,14 +98,14 @@ describe('allow to configure EOL', () => { // using toEqual because line endings matter in these tests expect( optimize(svg, { - js2svg: { eol: 'lf', pretty: true, indent: 2 }, + js2svg: { eol: 'crlf', pretty: true, indent: 2 }, }).data ).toEqual( - `\n \n\n` + `\r\n \r\n\r\n` ); }); - test('should respect EOL set to CRLF', () => { + test('should respect EOL set to line break', () => { const svg = ` @@ -127,7 +118,7 @@ describe('allow to configure EOL', () => { // using toEqual because line endings matter in these tests expect( optimize(svg, { - js2svg: { eol: 'crlf', pretty: true, indent: 2 }, + js2svg: { eol: '\r\n', pretty: true, indent: 2 }, }).data ).toEqual( `\r\n \r\n\r\n` From d874c0145f17c6c1507d4d8a8a081e911e27710c Mon Sep 17 00:00:00 2001 From: Adaline Valentina Simonian Date: Sat, 11 Sep 2021 09:57:34 -0700 Subject: [PATCH 4/7] Update js2svg eol tests to use destructuring Aligns test format with refactor made in main branch --- lib/svgo.test.js | 48 ++++++++++++++++++++++-------------------------- 1 file changed, 22 insertions(+), 26 deletions(-) diff --git a/lib/svgo.test.js b/lib/svgo.test.js index 3e23a8ac7..7922c725e 100644 --- a/lib/svgo.test.js +++ b/lib/svgo.test.js @@ -75,12 +75,11 @@ describe('allow to configure EOL', () => { `; + const { data } = optimize(svg, { + js2svg: { eol: 'lf', pretty: true, indent: 2 }, + }); // using toEqual because line endings matter in these tests - expect( - optimize(svg, { - js2svg: { eol: 'lf', pretty: true, indent: 2 }, - }).data - ).toEqual( + expect(data).toEqual( `\n \n\n` ); }); @@ -95,12 +94,11 @@ describe('allow to configure EOL', () => { `; + const { data } = optimize(svg, { + js2svg: { eol: 'crlf', pretty: true, indent: 2 }, + }); // using toEqual because line endings matter in these tests - expect( - optimize(svg, { - js2svg: { eol: 'crlf', pretty: true, indent: 2 }, - }).data - ).toEqual( + expect(data).toEqual( `\r\n \r\n\r\n` ); }); @@ -115,12 +113,11 @@ describe('allow to configure EOL', () => { `; + const { data } = optimize(svg, { + js2svg: { eol: '\r\n', pretty: true, indent: 2 }, + }); // using toEqual because line endings matter in these tests - expect( - optimize(svg, { - js2svg: { eol: '\r\n', pretty: true, indent: 2 }, - }).data - ).toEqual( + expect(data).toEqual( `\r\n \r\n\r\n` ); }); @@ -137,7 +134,8 @@ describe('allow to configure final newline', () => { `; - expect(optimize(svg).data).toMatchInlineSnapshot( + const { data } = optimize(svg); + expect(data).toMatchInlineSnapshot( `""` ); }); @@ -152,11 +150,10 @@ describe('allow to configure final newline', () => { `; - expect( - optimize(svg, { - js2svg: { finalNewline: true }, - }).data - ).toMatchInlineSnapshot(` + const { data } = optimize(svg, { + js2svg: { finalNewline: true }, + }); + expect(data).toMatchInlineSnapshot(` " " `); @@ -172,11 +169,10 @@ describe('allow to configure final newline', () => { `; - expect( - optimize(svg, { - js2svg: { finalNewline: true, pretty: true, indent: 2 }, - }).data - ).toMatchInlineSnapshot(` + const { data } = optimize(svg, { + js2svg: { finalNewline: true, pretty: true, indent: 2 }, + }); + expect(data).toMatchInlineSnapshot(` " From 21bb65fc26b500e7a6f73d17260df0c7044459e3 Mon Sep 17 00:00:00 2001 From: Adaline Valentina Simonian Date: Sat, 11 Sep 2021 10:36:02 -0700 Subject: [PATCH 5/7] Simplify EOL test cases, use toEqual for all tests EOL is no longer converted to lower case, so only `crlf` and `lf` are now supported as options, not `CRLF` or `LF` or any other case. --- lib/svgo.test.js | 39 +++++++++++++++++++-------------------- lib/svgo/js2svg.js | 15 +++++---------- 2 files changed, 24 insertions(+), 30 deletions(-) diff --git a/lib/svgo.test.js b/lib/svgo.test.js index 7922c725e..71102b33d 100644 --- a/lib/svgo.test.js +++ b/lib/svgo.test.js @@ -80,7 +80,7 @@ describe('allow to configure EOL', () => { }); // using toEqual because line endings matter in these tests expect(data).toEqual( - `\n \n\n` + '\n \n\n' ); }); @@ -99,11 +99,11 @@ describe('allow to configure EOL', () => { }); // using toEqual because line endings matter in these tests expect(data).toEqual( - `\r\n \r\n\r\n` + '\r\n \r\n\r\n' ); }); - test('should respect EOL set to line break', () => { + test('should default to LF line break for any other EOL values', () => { const svg = ` @@ -114,11 +114,11 @@ describe('allow to configure EOL', () => { `; const { data } = optimize(svg, { - js2svg: { eol: '\r\n', pretty: true, indent: 2 }, + js2svg: { eol: 'invalid', pretty: true, indent: 2 }, }); // using toEqual because line endings matter in these tests expect(data).toEqual( - `\r\n \r\n\r\n` + '\n \n\n' ); }); }); @@ -134,9 +134,10 @@ describe('allow to configure final newline', () => { `; - const { data } = optimize(svg); - expect(data).toMatchInlineSnapshot( - `""` + const { data } = optimize(svg, { js2svg: { eol: 'lf' } }); + // using toEqual because line endings matter in these tests + expect(data).toEqual( + '' ); }); @@ -151,12 +152,12 @@ describe('allow to configure final newline', () => { `; const { data } = optimize(svg, { - js2svg: { finalNewline: true }, + js2svg: { finalNewline: true, eol: 'lf' }, }); - expect(data).toMatchInlineSnapshot(` - " - " - `); + // using toEqual because line endings matter in these tests + expect(data).toEqual( + '\n' + ); }); test('should not add extra newlines when using pretty: true', () => { @@ -170,14 +171,12 @@ describe('allow to configure final newline', () => { `; const { data } = optimize(svg, { - js2svg: { finalNewline: true, pretty: true, indent: 2 }, + js2svg: { finalNewline: true, pretty: true, indent: 2, eol: 'lf' }, }); - expect(data).toMatchInlineSnapshot(` - " - - - " - `); + // using toEqual because line endings matter in these tests + expect(data).toEqual( + '\n \n\n' + ); }); }); diff --git a/lib/svgo/js2svg.js b/lib/svgo/js2svg.js index 181957b2f..49bf2c1a6 100644 --- a/lib/svgo/js2svg.js +++ b/lib/svgo/js2svg.js @@ -28,7 +28,7 @@ var defaults = { encodeEntity: encodeEntity, pretty: false, useShortTags: true, - eol: platformEOL, + eol: platformEOL === '\r\n' ? 'crlf' : 'lf', finalNewline: false, }; @@ -66,15 +66,10 @@ function JS2SVG(config) { this.config.indent = ' '; } - if (typeof this.config.eol === 'string' && this.config.eol) { - const configuredEOL = this.config.eol.toLowerCase(); - if (configuredEOL === 'lf') { - this.eol = '\n'; - } else if (configuredEOL === 'crlf') { - this.eol = '\r\n'; - } else { - this.eol = this.config.eol; - } + if (this.config.eol === 'crlf') { + this.eol = '\r\n'; + } else { + this.eol = '\n'; } if (this.config.pretty) { From 592c01fdf17ca38c87ce0b02fabb62dbd206225e Mon Sep 17 00:00:00 2001 From: Adaline Valentina Simonian Date: Sat, 11 Sep 2021 10:54:07 -0700 Subject: [PATCH 6/7] Remove toLowerCase() on --eol CLI option Achieves parity with non-CLI behaviour of not allowing different cases --- lib/svgo/coa.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/svgo/coa.js b/lib/svgo/coa.js index c772a2ad4..f304dbef0 100644 --- a/lib/svgo/coa.js +++ b/lib/svgo/coa.js @@ -118,8 +118,7 @@ async function action(args, opts, command) { } if (opts.eol != null) { - const eol = opts.eol.toLowerCase(); - if (eol !== 'lf' && eol !== 'crlf') { + if (opts.eol !== 'lf' && opts.eol !== 'crlf') { console.error( "error: option '--eol' must have one of the following values: 'lf' or 'crlf'" ); From c413409d3e68e53c78679dd1118cc5adffa145a7 Mon Sep 17 00:00:00 2001 From: Adaline Valentina Simonian Date: Sat, 11 Sep 2021 10:57:55 -0700 Subject: [PATCH 7/7] Merge if statements in --eol option test --- lib/svgo/coa.js | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/lib/svgo/coa.js b/lib/svgo/coa.js index f304dbef0..93cda6521 100644 --- a/lib/svgo/coa.js +++ b/lib/svgo/coa.js @@ -117,13 +117,11 @@ async function action(args, opts, command) { } } - if (opts.eol != null) { - if (opts.eol !== 'lf' && opts.eol !== 'crlf') { - console.error( - "error: option '--eol' must have one of the following values: 'lf' or 'crlf'" - ); - process.exit(1); - } + if (opts.eol != null && opts.eol !== 'lf' && opts.eol !== 'crlf') { + console.error( + "error: option '--eol' must have one of the following values: 'lf' or 'crlf'" + ); + process.exit(1); } // --show-plugins