[yaml] Serializable objects can also be output as JSON

[jwnimmer-tri]

• Requires [yaml] Opt-in to clang-format linting #18941.

Towards #15774. When shelling out to the python downloader, it will be run using the system Python which might not have any libraries, so to pass data back and forth we need to use JSON, not YAML.

---

This change is 

[jwnimmer-tri]

Possibly @rpoyner-tri could feature-review this, but I've also been peppering him with a lot of reviews lately. Other people (@xuchenhan-tri @sammy-tri @ggould-tri) might be interested in taking a feature review.

Whoever self-assigns first can take it.

[ggould-tri]

+@ggould-tri I've a bit of time now, I'll look in to this.

Reviewable status: LGTM missing from assignee ggould-tri(platform), needs at least two assigned reviewers, missing label for release notes

[ggould-tri]

; modest notes below.

Reviewed 3 of 9 files at r2, 11 of 12 files at r3, 4 of 4 files at r4, all commit messages.
Reviewable status: 3 unresolved discussions, needs at least two assigned reviewers (waiting on @jwnimmer-tri)

---

-- commits line 2 at r4:
minor: Assuming +(release notes: feature), this could be a little bit more explicit for the release engineer.

Suggestion:

[yaml] Yaml-serializable objects can also be output as JSON

---

common/yaml/yaml_io.h line 124 at r4 (raw file):

Note that there is no LoadJsonFile() function, because LoadYamlString() can
already load JSON data.

This statement is not reflected in the test cases.

I believe it in general, but with respect to the non-finite values I think there may be some format shear. Can you provide a test showing that json non-finites load as yaml non-finites? This could be a trivial round-trip test, assert x == LoadYamlString(SaveJsonString(x)) for a well-constructed x.

(There are also some other stupid and pedantic bits of shear around field names, but I don't see any reason to mention those in doxygen or to test them; reference https://metacpan.org/pod/JSON::XS#JSON-and-YAML if you wish to purchase a ticket to that particular flamewar)

Code quote:

Note that there is no LoadJsonFile() function, because LoadYamlString() can
already load JSON data.

---

common/yaml/yaml_read_archive.h line 285 at r4 (raw file):

    }
    // Figure out which variant<...> type we have based on the node's tag.
    const std::string_view tag = sub_node->GetTag();

nit: You burned away the const in every other case but this one.
Arguably this conforms to our style wrt const value args, so author's discretion.

Code quote:

const 

[jwnimmer-tri]

+@xuchenhan-tri for platform review (Wednesday schedule), please.

Reviewable status: 2 unresolved discussions, LGTM missing from assignee xuchenhan-tri(platform) (waiting on @ggould-tri and @xuchenhan-tri)

---

common/yaml/yaml_io.h line 124 at r4 (raw file):

Previously, ggould-tri wrote…

This statement is not reflected in the test cases.

I believe it in general, but with respect to the non-finite values I think there may be some format shear. Can you provide a test showing that json non-finites load as yaml non-finites? This could be a trivial round-trip test, assert x == LoadYamlString(SaveJsonString(x)) for a well-constructed x.

(There are also some other stupid and pedantic bits of shear around field names, but I don't see any reason to mention those in doxygen or to test them; reference https://metacpan.org/pod/JSON::XS#JSON-and-YAML if you wish to purchase a ticket to that particular flamewar)

This is a good point. I took the easy way out. I'll leave this thread open for you to resolve if you're OK with it.

---

common/yaml/yaml_read_archive.h line 285 at r4 (raw file):

Previously, ggould-tri wrote…

nit: You burned away the const in every other case but this one.
Arguably this conforms to our style wrt const value args, so author's discretion.

OK I'm content with "const all locals" but "don't const arguments" as a convention for these related stanzas.

[jwnimmer-tri]

Reviewable status: 1 unresolved discussion, LGTM missing from assignee xuchenhan-tri(platform) (waiting on @ggould-tri and @xuchenhan-tri)

---

common/yaml/yaml_io.h line 124 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

This is a good point. I took the easy way out. I'll leave this thread open for you to resolve if you're OK with it.

(I think that JSON rant is a little bit out of date now. The unicode string encoding and "/" escaping have since been fixed, at least as of yaml 1.2.2. To my first reading, the only remaining incompatibility is that 1024-char upper bound on map key strings.)

[ggould-tri]

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: LGTM missing from assignee xuchenhan-tri(platform) (waiting on @xuchenhan-tri)

---

common/yaml/yaml_io.h line 124 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

(I think that JSON rant is a little bit out of date now. The unicode string encoding and "/" escaping have since been fixed, at least as of yaml 1.2.2. To my first reading, the only remaining incompatibility is that 1024-char upper bound on map key strings.)

👍 Onward to Norway!

[xuchenhan-tri]

Platform

Reviewed 3 of 9 files at r2, 10 of 12 files at r3, 4 of 4 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: 1 unresolved discussion (waiting on @jwnimmer-tri)

---

common/yaml/yaml_write_archive.cc line 207 at r5 (raw file):

        // a sequence. We'll still fail-fast in case it ever does.
        throw std::logic_error(fmt::format(
            "SaveJsonString: Cannot save a YAML sequence with tag '{}' to JSON",

nit, this is covered by unit tests.

[jwnimmer-tri]

Reviewable status: 1 unresolved discussion (waiting on @xuchenhan-tri)

---

common/yaml/yaml_write_archive.cc line 207 at r5 (raw file):

Previously, xuchenhan-tri wrote…

nit, this is covered by unit tests.

That's right. The comment immediately above was my attempt to caution the read about what. WDYT?

[xuchenhan-tri]

Reviewable status: complete! all discussions resolved, LGTM from assignees ggould-tri(platform),xuchenhan-tri(platform) (waiting on @jwnimmer-tri)

---

common/yaml/yaml_write_archive.cc line 207 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

That's right. The comment immediately above was my attempt to caution the read about what. WDYT?

I see. I misinterpreted "can never" as "should never". I think this is fine.

Retracted.