rhai icon indicating copy to clipboard operation
rhai copied to clipboard

implementing From for Dynamic<serde_json::Value>?

Open jasongoodwin opened this issue 4 years ago • 4 comments

Hello,

I see that serde_json is a dependency under certain conditions - I'm wondering if you might be open to me implementing From for Dynamic<T: Value> if serde_json is enabled? This allows for the Union enum to interop with the serde_json::Value container. It might allow some better encapsulation if people don't want to couple their code to the Dynamic type too heavily - can use the serde_json Value and flip them into() Dynamics.

Let me know what you think - just an idea I'm having as I have some code in my codebase to deal with this but it would be really nice if From was implementing this right in Rhai. I can make a PR if you think it's a good idea but understand if it's not.

jasongoodwin avatar Jul 20 '21 18:07 jasongoodwin

Appreciate a PR!

You may simply put a new file under serde which impl From<Value> for Dynamic.

However, a catch:

Right now, serde_json is pulled in under the metadata feature, which can export functions metadata in JSON format.

Your feature of course has nothing to do with functions metadata, so maybe you should put it simply under the serde feature? It would be a little complicated because serde is not a declared feature in Rhai - it simply uses a dependency as a feature name. AFAIK there is currently no way for a feature to be named the same as some dependency, so there isn't a way for the serde feature to specify serde_json as a dependency...

To make things simple, you may keep it under the metadata feature - but then you're burdening the user with a lot of baggage (i.e. keeping metadata data) that he doesn't need.

An alternative is to create yet another feature - json - which simply pulls in serde_json. The pitfall is yet another feature... and there are already a lot of features...

Just some food for thoughts. I think in your case thinking about how the features will interact may be the biggest problem...

schungx avatar Jul 21 '21 00:07 schungx

Okay thanks - I'll think about it a bit and when I have a nice clean solution I'll contribute it back. Thanks for the insights.

jasongoodwin avatar Jul 26 '21 18:07 jasongoodwin

@jasongoodwin any progress on this?

schungx avatar Aug 30 '21 04:08 schungx

Hello! I'm definitely going to get to this, the project we're working on is focusing on some other areas currently, but I'll be returning here shortly and will nail this down. Give me a month or two, keep it open if that's cool, I'll fold it in once I'm able to dogfood it enough to be confident with how I'm contributing it.

jasongoodwin avatar Sep 01 '21 20:09 jasongoodwin

As a matter of fact, it seems like it is already possible to deserialize a Dynamic into serde_json::Value:

let value = json!({ "a": "b" });
println!("value: {value:?}");

let result: Dynamic = serde_json::from_value(value).unwrap();
println!("result: {result:?}");

let value2 = serde_json::to_value(&result).unwrap();
println!("value2: {value2:?}");

schungx avatar Nov 22 '22 08:11 schungx

OK, closing this for now.

schungx avatar Nov 30 '22 09:11 schungx

Hey sorry, just saw notifications from this. I had a bit of an unexpected pivot and haven't been on github much until more recently. Thanks for looking at this! Maybe something changed along the way or I missed something.

jasongoodwin avatar Jan 22 '23 00:01 jasongoodwin

Feel free to reopen if you find that it doesn't do what you intend.

schungx avatar Jan 24 '23 00:01 schungx