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

dumper: don't quote strings with # without need #521

Merged
merged 1 commit into from
Oct 21, 2019

Conversation

vcache
Copy link
Contributor

@vcache vcache commented Oct 21, 2019

According to spec a sharp symbol may be used within a key with plain style (i.e. without quotes) in certain conditions:

[130]   ns-plain-char(c) ::=   ( ns-plain-safe(c) - “:” - “#” )
                             | ( /* An ns-char preceding */ “#” )
                             | ( “:” /* Followed by an ns-plain-safe(c) */ )

That's handy when key contains URI like so:

uris:
  - http://example.com/page#anchor: some-example

@puzrin
Copy link
Member

puzrin commented Oct 21, 2019

  1. Term "fix" usually means something was broken (invalid output). As far as i understand, output without this update is valid. So, that's not fix but improvement.
  2. Need test to demonstrate use cases, why this change done.

&& c !== CHAR_COLON
&& c !== CHAR_SHARP;
&& (c !== CHAR_SHARP || prev && isPrintable(prev));
}
Copy link
Member

Choose a reason for hiding this comment

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

((c !== CHAR_SHARP) || (prev && isPrintable(prev)));

I'd use more scopes to simplify understanding.

@vcache vcache changed the title Dumper: fix spec compilance Dumper: enhance spec compilance Oct 21, 2019
@vcache
Copy link
Contributor Author

vcache commented Oct 21, 2019

Thanks for your review. You right, actually it is not a bug fix but proposition for more efficient usage of YAML specification. I've added some tests and made code more readable.

@puzrin
Copy link
Member

puzrin commented Oct 21, 2019

  • please move tests here with Issue/PR number
  • squash all and change commit name to "dumper: don't quote strings with # without need" - that's more informative.

Everything else seems ok.

@vcache vcache force-pushed the feature/dumper-spec-compilance branch from b5144b7 to b09def6 Compare October 21, 2019 20:59
@vcache vcache changed the title Dumper: enhance spec compilance dumper: don't quote strings with # without need Oct 21, 2019
@vcache vcache force-pushed the feature/dumper-spec-compilance branch from b09def6 to 667b3a1 Compare October 21, 2019 21:02
@puzrin puzrin merged commit d9fe622 into nodeca:master Oct 21, 2019
@puzrin
Copy link
Member

puzrin commented Oct 21, 2019

Thank you!

@vcache vcache deleted the feature/dumper-spec-compilance branch October 22, 2019 19:28
@Himura2la
Copy link
Contributor

Himura2la commented Jun 15, 2020

@vcache, The test does not test anything. You should not parse the sample YAML if you are testing how it's dumped. I changed the test like this:

test('Don\'t quote strings with # without need', function () {
  var required = readFileSync(require('path').join(__dirname, '/0521.yml'), 'utf8');
  var data = {
    'http://example.com/page#anchor': 'no#quotes#required',
    'parameter#fallback': 'quotes #required',
    'foo #bar': 'key is quoted'
  };
  var actual = yaml.safeDump(data);
  assert.equal(actual, required);
});

Now it fails. The http://example.com/page#anchor key is being quoted (because of a colon)

Himura2la added a commit to Himura2la/js-yaml that referenced this pull request Jun 15, 2020
Himura2la added a commit to Himura2la/js-yaml that referenced this pull request Jun 17, 2020
Himura2la added a commit to Himura2la/js-yaml that referenced this pull request Jun 17, 2020
Himura2la added a commit to Himura2la/js-yaml that referenced this pull request Jun 17, 2020
Himura2la added a commit to Himura2la/js-yaml that referenced this pull request Jun 17, 2020
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

Successfully merging this pull request may close these issues.

3 participants