Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

How to implement From<Option<T>> for Value where T is my custom value? #107

Closed
nappa85 opened this issue Aug 25, 2021 · 9 comments
Closed

How to implement From<Option<T>> for Value where T is my custom value? #107

nappa85 opened this issue Aug 25, 2021 · 9 comments

Comments

@nappa85
Copy link
Contributor

nappa85 commented Aug 25, 2021 •
edited
Loading

Let's say I have a field in my table, something like

status: CHAR(1) NOT NULL

status can have those values: A (active), D (deleted), Q (queued), E (executed).
I want to map the field to a Rust enum, I can easily do:

pub enum Status {
  Active,
  Deleted,
  Queued,
  Executed,
}

impl From<Status> for Value {
  fn from(s: Status) -> Value {
    Value::from(match s {
      Status::Active => "A",
      Status::Deleted => "D",
      Status::Queued => "Q",
      Status::Executed => "E",
    })
  }
}

and everything works fine.

But if my status is, instead

status: CHAR(1) DEFAULT NULL

In my Model I would have to use Option< Status>, and I cant impl From< Option< Status>> for Value, because neither Option or Value comes from my crate.

Maybe you could solve removing every impl From< Option< Something>> for Value and replacing it with a simple impl< T: Into< Value>> From< Option< T>> for Value, I don't know if there would be conflicts, but this way you unlock a lot of custom types potentials
@tyt2y3
Copy link
Member

tyt2y3 commented Aug 25, 2021 •
edited
Loading

They're defined in https://github.com/SeaQL/sea-query/blob/cfae4eab32fcd1a87103cffcf8760315f404656f/src/value.rs#L173

So you think

impl<T: Into<Value>> From<Option<T>> for Value

instead of

impl From<Option<$type>> for Value {

would help?

@nappa85
Copy link
Contributor Author

nappa85 commented Aug 25, 2021 •
edited
Loading

Yes, I've gone there only after opening the issue, and saw how you implemented Value...
This way you're locked by type, because for example bool values goes into Value::Bool and I can't see a generic to reconduce a None::< T> into the right Value variant.
A solution could be introduce Value::Null, this way the null value would be unique instead of typed.

To be honest, the same approach has been used by, for example, serde_json::Value, where variants aren't Option, and there is a Null variant to express the null value without knowing the type of the field.

I don't know if your approach has been forced by some requirements, but it's kind of less flexible.
How do you see it?

@tyt2y3
Copy link
Member

tyt2y3 commented Aug 25, 2021 via email

@nappa85
Copy link
Contributor Author

nappa85 commented Aug 25, 2021 via email

@tyt2y3
Copy link
Member

tyt2y3 commented Aug 25, 2021 via email •
edited
Loading

@nappa85
Copy link
Contributor Author

nappa85 commented Aug 25, 2021 via email

@tyt2y3
Copy link
Member

tyt2y3 commented Aug 25, 2021 via email •
edited
Loading

This was referenced Aug 25, 2021
Merged
Merged
@MuhannadAlrusayni MuhannadAlrusayni mentioned this issue Aug 28, 2021
Closed
@tyt2y3
Copy link
Member

tyt2y3 commented Aug 30, 2021

With #108 merged, is there still any other blocker?

@nappa85
Copy link
Contributor Author

nappa85 commented Aug 30, 2021

No, on this topic there are no blockers

@nappa85 nappa85 closed this as completed Aug 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@tyt2y3 @nappa85