Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

follow up on #470->#473: no quotes when not absolutely necessary #484

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 5 additions & 15 deletions lib/js-yaml/dumper.js
Expand Up @@ -190,18 +190,7 @@ function isPrintable(c) {

// Simplified test for values allowed after the first character in plain style.
function isPlainSafe(c) {
// Uses a subset of nb-char - c-flow-indicator - ":" - "#"
// where nb-char ::= c-printable - b-char - c-byte-order-mark.
return isPrintable(c) && c !== 0xFEFF
// - c-flow-indicator
&& c !== CHAR_COMMA
&& c !== CHAR_LEFT_SQUARE_BRACKET
&& c !== CHAR_RIGHT_SQUARE_BRACKET
&& c !== CHAR_LEFT_CURLY_BRACKET
&& c !== CHAR_RIGHT_CURLY_BRACKET
// - ":" - "#"
&& c !== CHAR_COLON
&& c !== CHAR_SHARP;
return isPrintable(c) && c !== 0xFEFF;
}

// Simplified test for values allowed as the first character in plain style.
Expand Down Expand Up @@ -262,7 +251,9 @@ function chooseScalarStyle(string, singleLineOnly, indentPerLevel, lineWidth, te
var shouldTrackWidth = lineWidth !== -1;
var previousLineBreak = -1; // count the first line correctly
var plain = isPlainSafeFirst(string.charCodeAt(0))
&& !isWhitespace(string.charCodeAt(string.length - 1));
&& !isWhitespace(string.charCodeAt(string.length - 1))
&& /\s#/.test(string) === false
&& /:(?:\s|$)/.test(string) === false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All checks should be done via .charCodeAt() for perfomance reasons.

Also, this lines are not descriptive - difficult to understand.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would comments help (apart from the fact that these are otherwise undesirable, see also my comments further below about the new insights about this)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, comments help in such cases. Or descriptive helper function names (like isWhiteSpace, hasCommentPattern)

But regexps should not be used - see any other part of code, there are no regexps.


if (singleLineOnly) {
// Case: no block styles.
Expand Down Expand Up @@ -296,7 +287,7 @@ function chooseScalarStyle(string, singleLineOnly, indentPerLevel, lineWidth, te
// in case the end is missing a \n
hasFoldableLine = hasFoldableLine || (shouldTrackWidth &&
(i - previousLineBreak - 1 > lineWidth &&
string[previousLineBreak + 1] !== ' '));
string[previousLineBreak + 1] !== ' '));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think it doesn't worth to change indent here if that's not related to your issue.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was eslint --fix (make fix) in action

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

=> don't use eslint --fix :)

There is local make lint with appropriate config, and it does not report error on this line.

}
// Although every style can represent \n without escaping, prefer block styles
// for multiline, since they're more readable and they don't add empty lines.
Expand Down Expand Up @@ -760,7 +751,6 @@ function writeNode(state, level, object, block, compact, iskey) {
state.dump = '!<' + state.tag + '> ' + state.dump;
}
}

return true;
}

Expand Down
145 changes: 145 additions & 0 deletions test/issues/0470.js
@@ -0,0 +1,145 @@
'use strict';

var assert = require('assert');
var yaml = require('../..');

test('should not unnecessarily apply quotes', function () {

var expected = 'url: https://github.com/nodeca/js-yaml\n';
var input = {
url: 'https://github.com/nodeca/js-yaml'
};

var actual = yaml.dump(input);
assert.strictEqual(actual, expected);

var roundTrip = yaml.safeLoad(actual);
assert.deepStrictEqual(roundTrip, input);
});

test('should not unnecessarily apply quotes - space then /\n at end of value', function () {

var expected = 'url: \'https://github.com/nodeca/js-yaml \'\n';
var input = {
url: 'https://github.com/nodeca/js-yaml '
};

var actual = yaml.dump(input);
assert.strictEqual(actual, expected);

var roundTrip = yaml.safeLoad(actual);
assert.deepStrictEqual(roundTrip, input);
});

test('should not unnecessarily apply quotes - space after colon', function () {

var expected = 'url: \'https: //github.com/nodeca/js-yaml\'\n';
var input = {
url: 'https: //github.com/nodeca/js-yaml'
};

var actual = yaml.dump(input);
assert.strictEqual(actual, expected);

var roundTrip = yaml.safeLoad(actual);
assert.deepStrictEqual(roundTrip, input);
});

/*
check for the removal of these dumper checks to deliver proper output

&& c !== CHAR_COMMA
&& c !== CHAR_LEFT_SQUARE_BRACKET
&& c !== CHAR_RIGHT_SQUARE_BRACKET
&& c !== CHAR_LEFT_CURLY_BRACKET
&& c !== CHAR_RIGHT_CURLY_BRACKET
// - ":" - "#"
&& c !== CHAR_COLON
&& c !== CHAR_SHARP;
*/
test('should not unnecessarily apply quotes - : colon, # sharp, comma and []{}', function () {

var expected = [
'sharp1: C#123',
'sharp2: C# 123',
"sharp3: 'C #123'",
'sharp4: C123#',
"sharp5: '#C123'",
"sharp6: '#C123#'",
'colon1: C:123',
"colon2: 'C: 123'",
'colon3: C :123',
"colon4: 'C123:'",
"colon5: ':C123'",
"colon6: ':C123:'",
'comma1: C,123',
'comma2: C, 123',
'comma3: C ,123',
'comma4: C123,',
"comma5: ',C123'",
"comma6: ',C123,'",
'sqbr1: C[1]23',
'sqbr2: C [1]23',
"sqbr3: '[C] 123'",
'sqbr4: C12[3]',
"sqbr5: '[C]123'",
"sqbr6: '[C]12[3]'",
"sqbr7: '[]C123[]'",
'sqbr8: C [123]',
'cbr1: C{1}23',
'cbr2: C {1}23',
"cbr3: '{C} 123'",
'cbr4: C12{3}',
"cbr5: '{C}123'",
"cbr6: '{C}12{3}'",
"cbr7: '{}C123{}'",
'cbr8: C {123}',
''
].join('\n');
var input = {
sharp1: 'C#123',
sharp2: 'C# 123',
sharp3: 'C #123',
sharp4: 'C123#',
sharp5: '#C123',
sharp6: '#C123#',

colon1: 'C:123',
colon2: 'C: 123',
colon3: 'C :123',
colon4: 'C123:',
colon5: ':C123',
colon6: ':C123:',

comma1: 'C,123',
comma2: 'C, 123',
comma3: 'C ,123',
comma4: 'C123,',
comma5: ',C123',
comma6: ',C123,',

sqbr1: 'C[1]23',
sqbr2: 'C [1]23',
sqbr3: '[C] 123',
sqbr4: 'C12[3]',
sqbr5: '[C]123',
sqbr6: '[C]12[3]',
sqbr7: '[]C123[]',
sqbr8: 'C [123]',

cbr1: 'C{1}23',
cbr2: 'C {1}23',
cbr3: '{C} 123',
cbr4: 'C12{3}',
cbr5: '{C}123',
cbr6: '{C}12{3}',
cbr7: '{}C123{}',
cbr8: 'C {123}'
};

var actual = yaml.dump(input);
assert.strictEqual(actual, expected);

var roundTrip = yaml.safeLoad(actual);
assert.deepStrictEqual(roundTrip, input);
});
4 changes: 2 additions & 2 deletions test/units/dump-scalar-styles.js
Expand Up @@ -39,9 +39,9 @@ suite('Scalar style dump:', function () {

test('disallows flow indicators inside flow collections', function () {
assert.strictEqual(yaml.safeDump({ quote: 'mispell [sic]' }, { flowLevel: 0 }),
"{quote: 'mispell [sic]'}\n");
'{quote: mispell [sic]}\n');
assert.strictEqual(yaml.safeDump({ key: 'no commas, either' }, { flowLevel: 0 }),
"{key: 'no commas, either'}\n");
'{key: no commas, either}\n');
});
});

Expand Down