Skip to content
This repository has been archived by the owner on Mar 25, 2024. It is now read-only.

Use yaml-rust 0.4 #83

Merged
merged 2 commits into from Nov 22, 2017
Merged

Use yaml-rust 0.4 #83

merged 2 commits into from Nov 22, 2017

Conversation

vroland
Copy link
Contributor

@vroland vroland commented Nov 22, 2017

As yaml-rust 0.4 got publisched on crates.io recently, it would be nice to use it for serde-yaml. There are some some fixes (especially for the emitter) I'd really like use with serde.

@killercup killercup mentioned this pull request Nov 22, 2017
Copy link
Owner

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Thanks!

@dtolnay dtolnay merged commit 9b25009 into dtolnay:master Nov 22, 2017
@dtolnay
Copy link
Owner

dtolnay commented Nov 22, 2017

@vroland I haven't looked through the yaml-rust changes in detail so I need your advice -- is this safe to publish as 0.7.3 of serde-yaml or does the input/output change in a way that requires a major version from us?

@@ -94,7 +94,7 @@ fn test_map() {
let yaml = unindent(r#"
---
x: 1
y: 2"#);
"y": 2"#);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is wrong and there is some bug... @dtolnay

Copy link
Owner

Choose a reason for hiding this comment

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

This is from chyh1990/yaml-rust#72.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is probably an issue with the emitter. But this is more of a cosmetic issue, as it is still valid YAML with the same meaning.

Copy link

Choose a reason for hiding this comment

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

this is intended to keep any other yaml1.1 (not 1.2) parser from interpreting this as a boolean. I am currently unaware whether this ought to be treated differently when the value is a key.

Copy link

Choose a reason for hiding this comment

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

I have tried the yaml parser in ruby 2.3, 'Y', 'y', 'N', 'n' is not interpreted as boolean in both keys and values. But 'YES', 'yes', 'NO', 'no' is boolean in ruby. PyYAML also works like this.

I know it is violating YAML spec, the using 'y' in key is a common case.

Copy link

Choose a reason for hiding this comment

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

the ruby parses an pyyaml are all 1.1 parsers

@vroland
Copy link
Contributor Author

vroland commented Nov 22, 2017

As far as I am aware these breaking changes are only present in parser event handling, which is not exposed by serde-yaml. But ScanError struct contains an additional field "mark". If this is a problem we need a higher version.

@Rufflewind
Copy link

It looks v0.4 changes the rendering for certain cases. Previously, I got:

blah: 
  {a: 1, b: 2, c: 3}: 1.2345

Now I get:

blah:
  ? "a": 1
    b: 2
    c: 3
  : 1.2345

I found it confusing that the major version did not increase. (Also, the quoting around just "a" but none of the other keys looks weird.)

@hoodie
Copy link

hoodie commented Dec 5, 2017

@Rufflewind: semver allows this. Below 1.0 every 0.X version may contain breaking changes.
The emitter has been replaced in this version. I was not aware of this behavior before, but it appears to be valid yaml. The 0.3.x version used to emit invalid yaml in certain edgecases. Seems we should look into this again after all, but at least it's parsable now :P

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

Successfully merging this pull request may close these issues.

None yet

6 participants