Skip to content

Add serde from/to string impl#53

Draft
james-allan-lloyd wants to merge 1 commit intosaphyr-rs:masterfrom
james-allan-lloyd:add-serde-from-to-string
Draft

Add serde from/to string impl#53
james-allan-lloyd wants to merge 1 commit intosaphyr-rs:masterfrom
james-allan-lloyd:add-serde-from-to-string

Conversation

@james-allan-lloyd
Copy link

Hi,

Thanks for the great YAML parsing library! I needed to do some parsing of Yaml into tera::Values, so instead of just doing a translation from saphyr::Yaml to tera::Value, I instead wrote a serde desrializer 😅. I figured this might also be interesting to the saphyr project due to #1. I also implemented basic serialization, although I don't use it in my project.

I hope this is the correct way to share this. I'm relatively new to both Rust and Open Source contribution, so happy to accept feedback.

@Ethiraric
Copy link
Contributor

Ethiraric commented Jun 26, 2025

Thank you very much for this huge contribution. This has been a highly requested feature for the library.
This was my most recent focus for saphyr but I have not yet completed the implementation.


I'll put this PR on hold for a tiny bit. There is a lot here to unpack, and I think I'll make a few changes directly on your branch before I come back to you.
I haven't fully checked the code but some things stood out:

  • I want most of the heavy testing to be in the tests/ folder rather than having a test folder/mod in src/.
  • We will get rid of the Regex. It is pretty much over-complicated for what it does. bool parsing has changed in YAML1.2 and is much simpler than in YAML1.1. Namely, NO, yes and the like no longer are valid boolean representations.
  • There is code to be deduplicated between serde-saphyr and serde.
  • I have started changing the emitter so it can support events here. The doccomment is missing an example, but some basic tests lay at the end of the file. We should use it for serialization.
  • I avoid test parsing / emitting through asserting string equality, but rather by roundtripping (str -> Yaml -> str -> Yaml and compare both Yaml). String equality is the proper way of testing presentation options in the emitter, but I haven't yet come to this.
  • I'd like to have the crate tested against the yaml-test-suite.
  • Your doccomments should use the /// syntactic sugar.
  • Your name is missing from the authors in Cargo.toml :)

I hope this is the correct way to share this. I'm relatively new to both Rust and Open Source contribution, so happy to accept feedback.

It is! Welcome to the world of Open Source! 👋


I'll set this PR as a draft, fix a few of the above, and come back to you. Does that work for you? I'll make this my priority on this crate. I expect to have done a bit Sunday evening (EU time) at the latest.

@Ethiraric Ethiraric marked this pull request as draft June 26, 2025 20:24
@james-allan-lloyd
Copy link
Author

Hi, great that I can contribute! I know there were rough edges, but I wanted to put it out there in case someone else was already working on it :)

I can redo the tests in the roundtrip style, if you want. Probably can also run against the yaml test suite by deserializing to serde_json::Value and writing json to compare, right?

@Ethiraric
Copy link
Contributor

I'm sorry, IRL has been hectic and I haven't had the energy to fully come back to this. Thank you very much for your interest!


Probably can also run against the yaml test suite by deserializing to serde_json::Value and writing json to compare, right?

What I would rather do is compare the serde implementation with the saphyr implementation. Something along the lines of:

fn f(input: &s) {
  match Yaml::load_from_str(s) {
    Ok(saphyr) -> assert_eq!(saphyr, saphyr_serde::from_str(s).unwrap(),
    Err(_) -> assert!(saphyr_serde::from_str(s).is_err())
  }
}

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.

2 participants