Option other basic GValue types?

[criminosis]

In https://github.com/wolf4ood/gremlin-rs/blob/2e18b3cb09217a1c7f9645f98ce98a90a69acfab/gremlin-client/src/structure/value.rs#L410

it seems Option is supported but omitted other basic types like Int32, Int64, etc.

Could those be added?

[wolf4ood]

Hi @criminosis

Thanks for raising this issue. Sure they can be added. I will mark as enhancement and work on this for the next release :)

[criminosis]

Thanks! FWIW I played with a local checkout of the code and I was getting a stack overflow when I added:

impl_try_from_option!(i32);
impl_try_from_option!(i64);

when it encountered a node that had a value (the lack of the property parsing into a None worked as expected)

I ended up doing this:

impl std::convert::TryFrom<GValue> for Option<i64> {
    type Error = crate::GremlinError;

    fn try_from(value: GValue) -> GremlinResult<Self> {
        if let GValue::Null = value {
            return Ok(None);
        }

        let res: i64 = value.try_into()?;
        Ok(Some(res))
    }
}

impl std::convert::TryFrom<GValue> for Option<i32> {
    type Error = crate::GremlinError;

    fn try_from(value: GValue) -> GremlinResult<Self> {
        if let GValue::Null = value {
            return Ok(None);
        }

        let res: i32 = value.try_into()?;
        Ok(Some(res))
    }
}

That seems to now work but other than the type hint of each res being a i64 or a i32 it doesn't feel like that should be radically different from what was being generated by the macro?

So tweaking with the original macro arrived at this:

macro_rules! impl_try_from_option {
    ($t:ty) => {
        impl std::convert::TryFrom<GValue> for Option<$t> {
            type Error = crate::GremlinError;

            fn try_from(value: GValue) -> GremlinResult<Self> {
                if let GValue::Null = value {
                    return Ok(None);
                }
                let res: $t = value.try_into()?;
                Ok(Some(res))
            }
        }
    };
}

impl_try_from_option!(String);
impl_try_from_option!(i32);
impl_try_from_option!(i64);

Which seems to work for the new i32 & i64 and doesn't seem to have broken Option<String> as far as I can tell. The addition of : $t to the macro's res to hint I think to what needs the GValue needs to be parsed into? And then I had to wrap the value in a Some before returning.

I could spin up a PR if you'd like, but I'm fairly green in Rust still so not sure if what I've done is the "right" Rust way or I just cobbled something together that coincidentally seems to work because it's missing edge cases.

[wolf4ood]

Hi @criminosis

yes it seems correct your patch. Probably it was going to stackoverflow also with String.

If you have time yes submit a PR :)