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

Dumping add quote to string even it is not necessary #470

Closed
joshuaavalon opened this issue Feb 25, 2019 · 11 comments
Closed

Dumping add quote to string even it is not necessary #470

joshuaavalon opened this issue Feb 25, 2019 · 11 comments

Comments

@joshuaavalon
Copy link

As title.

Test Code

const jsYaml = require("js-yaml");
const fs = require("fs");
const yamlStr = jsYaml.dump({ url: "https://github.com/nodeca/js-yaml" });
fs.writeFileSync("foo.yml", yamlStr, { encoding: "utf8" });

Expected Result

url: https://github.com/nodeca/js-yaml

Actual Result

url: 'https://github.com/nodeca/js-yaml'

Problem

plain = plain && isPlainSafe(char);

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;
}

All of these characters ( except0xFEFF ) are safe inside string. Only colon cannot be followed by space or placed at the end of line.

@diberry
Copy link
Contributor

diberry commented Feb 26, 2019

This is the same as #466. Perhaps we can have a new setting to handle the quoting issue?

@puzrin
Copy link
Member

puzrin commented Feb 26, 2019

@joshuaavalon "unquoting" quality is limited only by authors time. If anyone has time to improve it - PR is welcome.

@diberry
Copy link
Contributor

diberry commented Feb 26, 2019

@puzrin I'll make a PR this weekend, if no one does it before then.

@diberry
Copy link
Contributor

diberry commented Mar 2, 2019

PR 454 seems related

@diberry
Copy link
Contributor

diberry commented Mar 3, 2019

@puzrin Are these tests all valid/correct?

`test.only('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);
});
test.only('should not unnecessaryly apply quotes - ', function () {

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

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.only('should not unnecessaryly apply quotes - space after colon', 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);
});
`

@puzrin
Copy link
Member

puzrin commented Mar 3, 2019

Could you format you message and add proper lang type for highlight?

GerHobbelt added a commit to GerHobbelt/js-yaml that referenced this issue Apr 15, 2019
GerHobbelt added a commit to GerHobbelt/js-yaml that referenced this issue Apr 15, 2019
GerHobbelt added a commit to GerHobbelt/js-yaml that referenced this issue Apr 15, 2019
…ll round trip tests pass.

# Conflicts:
#	test/issues/0470.js
GerHobbelt added a commit to GerHobbelt/js-yaml that referenced this issue Apr 15, 2019
# Conflicts:
#	test/issues/0470.js
GerHobbelt added a commit to GerHobbelt/js-yaml that referenced this issue Apr 16, 2019
…e rendered output is ambiguous and hence erroneous YAML.
GerHobbelt added a commit to GerHobbelt/js-yaml that referenced this issue Apr 16, 2019
@kchen-shanghai
Copy link

Having a same problem here. Since only colon and hash characters are limited in flow scalars, is a PR still welcome by now?

@Giszmo
Copy link

Giszmo commented Jul 22, 2020

@ZEROPC I would appreciate a fix but but not speaking for the project. Crossing fingers :D

@rlidwka
Copy link
Member

rlidwka commented Dec 10, 2020

Fixed in db3f529 (in dev branch currently, to be released later).

@rlidwka rlidwka closed this as completed Dec 10, 2020
@mgiamberardino
Copy link

I'm experiencing a related issue but not entirely sure is the same one.

We have some cases where the object is like:
{ "CC#": "some data" }

And right now this is being confused with a comment in the library. Is this fix related or should I create a new issue?

@rlidwka
Copy link
Member

rlidwka commented Dec 25, 2020

And right now this is being confused with a comment in the library. Is this fix related or should I create a new issue?

This fix is not related, so it's a new issue.

But I'm not entirely sure what you're talking about. # symbol is required to have a space/tab/newline before it to be a comment.

If you just put CC#, it's not parsed as a comment:

> require('js-yaml').dump({"CC#": "some data"})
'CC#: some data\n'

> require('js-yaml').load('CC#: some data\n')
{ 'CC#': 'some data' }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants