#470 fix #473
#470 fix #473
Conversation
|
|
||
| assert.strictEqual(actual, expected); | ||
| }); | ||
| test('should not unnecessaryly apply quotes - ', function () { |
There was a problem hiding this comment.
This test looks like previous, with no difference.
| assert.strictEqual(actual, expected); | ||
| }); | ||
|
|
||
| test('should not unnecessaryly apply quotes - space then /\n at end of value', function () { |
There was a problem hiding this comment.
Don't see difference with first test
|
|
||
| assert.strictEqual(actual, expected); | ||
| }); | ||
| test('should not unnecessaryly apply quotes - space after colon', function () { |
There was a problem hiding this comment.
Don't see difference with first test
| } | ||
| ); | ||
|
|
||
| assert.strictEqual(actual, expected); |
There was a problem hiding this comment.
That's not enougth. Need to check that after load back result will be exact as initial
| plain = plain && isPlainSafe(char); | ||
| if (!isPlainSafe(char)) { | ||
| plain = plain && !isWhitespace(string.charCodeAt(i + 1)); | ||
| } |
There was a problem hiding this comment.
This is a very cryptic way to express "if plainsafe and next is not whitespace". Very deficult to understand real intent. Please, reorganize somehow.
| @@ -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))); | |||
There was a problem hiding this comment.
isWhitespace(string.charCodeAt(i + 1)) - missed length check, to avoid out of bounds access at [i + 1]. Such thing will trash JIT optimisations.
There was a problem hiding this comment.
Should it check next or prev char?
{ test: 'C #123' } - make sure this will be quoted.
| // - ":" - "#" | ||
| && c !== CHAR_COLON | ||
| && c !== CHAR_SHARP; | ||
| && c !== CHAR_COLON; |
There was a problem hiding this comment.
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
|
https://github.com/diberry/js-yaml/pull/1 should answer many of the comments above. That pullreq introduces round-trip tests and has more tests. |
|
@GerHobbelt i think it's better to make your PR as independent, and to this repo. Then we can discuss details. |
|
Close as incomplete/invalid. As been noted in #484 (comment), this will break flow style of dump. |
…e rendered output is ambiguous and hence erroneous YAML.
No description provided.