ValueType trait should have a non-panic-ing method

[nappa85]

Sorry for opening so many issues in those days...

Actually the code looks like this:

pub trait ValueType: ValueTypeDefault {
    fn unwrap(v: Value) -> Self;

    fn type_name() -> &'static str;
}

What I'm proposing is to change it into something like this:

pub trait ValueType: ValueTypeDefault {
    fn try_into(v; Value) -> Result<Self, SomeError> {
        Ok(Self::unwrap())
    }

    #[deprecated]
    fn unwrap(v: Value) -> Self;

    fn type_name() -> &'static str;
}

It's never good having panic-ing code around, it's better to have an error type, even if unused on basic operations.
This impl permits retrocompatibility with actual code but gives a deprecation notice, later it should become

pub trait ValueType: ValueTypeDefault {
    fn try_into(v; Value) -> Result<Self, SomeError>;

    #[deprecated]
    fn unwrap(v: Value) -> Self {
        Self::try_into().unwrap()
    }
    fn type_name() -> &'static str;
}

Forcing people to switch to the new impl, meanwhile the crate code should have migrated to the Result version of the trait.

Another suggestion, to write and request less code, could be

pub trait ValueTypeDefault {
    fn default() -> Self;
}

impl<T: ValueTypeDefault> for Option<T> {
    fn default() -> Self {
        None
    }
}

This would also solve the problem with foreign types implementation (I can't impl ValueTypeDefault for Option<Something> because I don't own Option nor ValueTypeDefault), but opens a philosophical question about None being always the correct default for Option<T>

[tyt2y3]

Yes I agree to have a try_into method that does not panic.
But I think we don't need to deprecate the unwrap method?

[nappa85]

Maybe I'm getting it wrong, but the unwrap method isn't used only internally?
In which situation a developer using this crate should use the unwrap method?
And, in the unfortunate situation where this is needed and he can't return a Result, why calling .try_into().unwrap() isn't viable?

I feel like using panic-ing code is a bad pratice and shoudn't be suggested, anyway it's a philosophical argument and the deprecation isn't the core of the issue

[tyt2y3]

Yes. I think if people are using SeaQuery directly, then they'd unwrap() on a Value.

let a: i32 = val.unwrap();

And I think it's just like using Option and unwrap() when they are sure that the Option is not None.

[nappa85]

Yes, it's not a best practice but I understand, the difference is that you can check an Option isn't None before unwrapping, can you check a Value before unwrapping?

[tyt2y3]

Yes, there are some methods is_json is_date_time is_decimal etc, but it does not cover all possible types indeed.
Perhaps for consistency I should add more is_* to cover all types.

[nappa85]

Ok, like serde_json::Value, there it has also some as_json, as_date_time, etc... methods that returns Option, can be useful to implement, this way people can use directly if let istead of if and then inside unwrap.
But what I'm talking about aren't those predictable problems, but the ones given for example by a wrong typing.
Maybe my custom type manages only Char(1) A, B and C and from db comes a D, having an error instead of a panic would be great here.
Same if I ask for a basic type, like i32, but the field is nullable and I haven't used Option, unwrap would panic on None...

Ok, you can say those are development problems, I see the panic, fix the error and run again, but if unfortunately I don't find the error before deploying, I'll have panics in production, that isn't a good thing...

What I'm trying to say is that developers often do bad things, but it must their fault, not crate's fault for not having given them a way to enforce best practices.

At the end we ended up in philosophical...

[tyt2y3]

Yes sure, this is quite philosophical. I agree with all your points indeed. Having unpredictable panics sounds pretty bad. I'd agree with this proposal.

I personally tend to be less opinioned. For example, that's why SeaQuery has support for sqlx, as well as the postgres crate and rusqlite at the same time.