Add support for parsing to dynamic - #11
Conversation
|
Hi @lpil! Can I bump you about this PR? |
lpil
left a comment
lpil
left a comment
There was a problem hiding this comment.
Hello! Thank you for this! Will be super useful.
I've left some notes inline. One of the main things is that new_primitive_decoder is being used a lot instead of the regular decode API. new_primitive_decoder is for decoding primitives via FFI, it's not needed to compose decoders, they already compose!
| } | ||
| } | ||
|
|
||
| /// Convert a parsed TOML document into a `Dynamic`. |
There was a problem hiding this comment.
Fill in this documentation with what it is for please 🙏
There was a problem hiding this comment.
I filled this out for both to_dynamic and parse_to_dynamic. Let me know what you think :)
| } | ||
|
|
||
| /// A convenience for parsing a TOML document and immediately converting it to a Dynamic | ||
| pub fn parse_dynamic(input: String) -> Result(Dynamic, ParseError) { |
There was a problem hiding this comment.
Same here, more documentation please.
Also let's call it parse_to_dynamic, to make it a bit clearer 🙏
|
@lpil I've addressed your feedback and given this a rebase, when you have a chance! |
lpil
left a comment
lpil
left a comment
There was a problem hiding this comment.
Awesome, thank you! I've left some very small notes inline 🙏
|
|
||
| @external(erlang, "tom_ffi", "infinity_to_dynamic") | ||
| @external(javascript, "./tom_ffi.mjs", "infinity_to_dynamic") | ||
| fn infinity_to_dynamic(infinity: InfinityValue) -> Dynamic |
There was a problem hiding this comment.
Rather than create new types and making them internal you can use a bool as the argument instead
There was a problem hiding this comment.
I think we can still use sign though yeah?
| } | ||
|
|
||
| assert decode.run(dynamic, decoder) | ||
| == Error([decode.DecodeError("DateTime with offset", "Dict", ["a"])]) |
There was a problem hiding this comment.
I don't love that this says it found a dict? But I don't think there's a better way to say that we found a DateTime with a local offset with the decoder API.
There was a problem hiding this comment.
Dynamic is structural, and there's no dedicated date time type, so it'll be a dict.
lpil
left a comment
lpil
left a comment
There was a problem hiding this comment.
Brilliant, thank you. Few tiny things inline.
Co-authored-by: Louis Pilfold <louis@lpil.uk>
Co-authored-by: Louis Pilfold <louis@lpil.uk>
2 participants
Fixes #10
This PR adds the ability to either parse directly to a
dynamic(parse_dynamic) or convert an existing TOML document to a dynamic (to_dynamic). I had to do some dynamic building like we discussed on Discord, so hopefully this matches with what we discussed 😅. I've also added decoders for all custom types, so that callers do not have to use the known forms themselves if they don't want to.I've also added a JS test target to the CI, since there's FFI involved now.
Let me know if you'd like anything fixed up!