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

#470 fix #473

Closed
wants to merge 1 commit 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
18 changes: 6 additions & 12 deletions lib/js-yaml/dumper.js
Original file line number Diff line number Diff line change
Expand Up @@ -194,14 +194,7 @@ function isPlainSafe(c) {
// 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;
&& c !== CHAR_COLON;
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to cover this change with tests. For example, check dumping (and load back) of this strings:

C#, C#123, C #123, C# 123

}

// Simplified test for values allowed as the first character in plain style.
Expand Down Expand Up @@ -272,7 +265,7 @@ function chooseScalarStyle(string, singleLineOnly, indentPerLevel, lineWidth, te
if (!isPrintable(char)) {
return STYLE_DOUBLE;
}
plain = plain && isPlainSafe(char);
plain = plain && (isPlainSafe(char) && !isWhitespace(string.charCodeAt(i + 1)));
Copy link
Member

@puzrin puzrin Mar 10, 2019

Choose a reason for hiding this comment

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

isWhitespace(string.charCodeAt(i + 1)) - missed length check, to avoid out of bounds access at [i + 1]. Such thing will trash JIT optimisations.

Copy link
Member

Choose a reason for hiding this comment

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

Should it check next or prev char?

{ test: 'C #123' } - make sure this will be quoted.

}
} else {
// Case: block styles permitted.
Expand All @@ -291,12 +284,14 @@ function chooseScalarStyle(string, singleLineOnly, indentPerLevel, lineWidth, te
} else if (!isPrintable(char)) {
return STYLE_DOUBLE;
}
plain = plain && isPlainSafe(char);
if (!isPlainSafe(char)) {
plain = plain && !isWhitespace(string.charCodeAt(i + 1));
}
Copy link
Member

Choose a reason for hiding this comment

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

This is a very cryptic way to express "if plainsafe and next is not whitespace". Very deficult to understand real intent. Please, reorganize somehow.

}
// in case the end is missing a \n
hasFoldableLine = hasFoldableLine || (shouldTrackWidth &&
(i - previousLineBreak - 1 > lineWidth &&
string[previousLineBreak + 1] !== ' '));
string[previousLineBreak + 1] !== ' '));
}
// 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 +755,6 @@ function writeNode(state, level, object, block, compact, iskey) {
state.dump = '!<' + state.tag + '> ' + state.dump;
}
}

return true;
}

Expand Down
46 changes: 46 additions & 0 deletions test/issues/0470.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
'use strict';

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

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

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

assert.strictEqual(actual, expected);
Copy link
Member

Choose a reason for hiding this comment

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

That's not enougth. Need to check that after load back result will be exact as initial

});
test('should not unnecessaryly apply quotes - ', function () {
Copy link
Member

Choose a reason for hiding this comment

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

This test looks like previous, with no difference.


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

var obj = {};
obj['url'] = 'https://github.com/nodeca/js-yaml';

var actual = yaml.dump(obj);

assert.strictEqual(actual, expected);
});

test('should not unnecessaryly apply quotes - space then /\n at end of value', function () {
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 see difference with first test


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

var obj = {};
obj['url'] = 'https://github.com/nodeca/js-yaml ';

var actual = yaml.dump(obj);

assert.strictEqual(actual, expected);
});
test('should not unnecessaryly apply quotes - space after colon', function () {
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 see difference with first test


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

assert.strictEqual(actual, expected);
});