Parsing I64 and U64 number properly

hjson / hjson-rust

Hjson for Rust

https://hjson.github.io/

MIT License

102 stars 33 forks source link

Parsing I64 and U64 number properly #9

Closed zonyitoo closed 5 years ago

zonyitoo commented 5 years ago

This is a PR for #8 .

Removed unused macro try_or_invalid.
Parse number into i64 or u64 if possible.

dqsully commented 5 years ago

Thanks for submitting this PR, it looks really good. I just have one question: does this PR make any breaking changes to number parsing? Like if someone wanted an f64 from an integer string, would they still get an f64 without errors? Or will they have to change their code to accommodate for all three possibilities? I'm not familiar with the older Serde code so I don't know.

Also, if you want to use a newer version of Serde, I tried porting the official Serde JSON plugin for Hjson here. I believe I got it working overall, although I've never used it in production, so use it at your own risk.

zonyitoo commented 5 years ago

does this PR make any breaking changes to number parsing?

Yes, it is a breaking change.

But it is very simple to be compatible with the old behavior, just modify Value::as_f64 to be able to cast f64 from Value::I64 and Value::U64.

zonyitoo commented 5 years ago

But it will still be a breaking change. If someone match the Value directly without as_f64(), then he will definitely get error after updating.

So I suggest to make a major version bump for this, to ensure that no existing code will break unexpectedly.

zonyitoo commented 5 years ago

Glad to see you have ported serde's JSON plugin to hjson, that's awesome. I would prefer to use your version if it is ready, because I am now migrating my project that was using serde-json to hjson-rust.

dqsully commented 5 years ago

The goal with that port was to fix #6, but I eventually forgot about it. I'll get your changes published at some point today, and then I'll see if I can merge the 150-or-so new commits from serde-json into my port. Hopefully I'll finally be able to close #6 and release serde-hjson v1.0! Either way, my code does pass all the core hjson test cases, so it should be ready.