-
-
Notifications
You must be signed in to change notification settings - Fork 192
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.
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
Introduce Nullable trait to permit custom Option<T> #112
Conversation
Brilliant! I have stumbled upon the awkardness of impling traits for Option for quite some time... |
I've evolved the PR to remove also the various |
Sorry, not trying to nitpick, but can we use impl<T> ValueType for Option<T>
where T:
ValueType + Nullable { inplace of impl<T: ValueType + Nullable> ValueType for Option<T> { |
Yes, not a problem, I personally prefer the compact form when there are few constraints |
Thank you ) |
} | ||
|
||
fn type_name() -> &'static str { | ||
//concat!("Option<", T::type_name(), ">") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Um... so you think the type name shouldn't be Option<_>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably it should, but I failed to find a way to generate a static string from a method calling another method... And then I forgot about it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe whit const trait fn stabilized it's possible, I don't know
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking https://doc.rust-lang.org/std/any/fn.type_name.html could have been a solution, but looking at the example I saw why it can't be
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this type_name
is mainly for diagnostic purpose. May be we wouldn't mind generating the string dynamically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you change the return type to Cow<'static, str>
you can still return static str in normal types and allocate String only in options
As discussed in Issue 107, the introduction of a Nullable trait permits to retrive the correct Value variant for the Null type, therefore to add a generic
impl From<Option<T>> for Value where T: Into<Value> + Nullable
, permitting to use optional custom types inside Models