Add as_borrow() generic method for Value #153
Conversation
There was a problem hiding this comment.
It allows users to use e.g. some_fn(try!(value.as_borrow())) instead of some_fn(try!(value.as_string().ok_or(SomeCustomError)))
I'm not sure that is sufficient motivation. Going overly generic with the as_borrow way sounds like it'll just make for unreadable error messages. Couldn't we just breaking-change the existing as_* methods to yield a Result<T, ValueType>?
| /// Just like `std::borrow::Borrow<Value>` but borrowing may fail. | ||
| pub trait TryBorrow<'a>: 'a + ::std::marker::Sized { | ||
| /// Returns reference to internal data if type matches. | ||
| fn try_borrow(value: &'a Value) -> Result<Self, TypeError>; |
There was a problem hiding this comment.
this trait could include the try_borrow_mut method
There was a problem hiding this comment.
The reason is to mimic Borrow and BorrowMut. I'm not even sure whether it would be possible to have them together in one trait, since one is defined for &'a T and the other for &'a mut T...
However, I'm open to put them together, if it's possible and advantageous.
| } | ||
| } | ||
|
|
||
| impl<'a> TryBorrow<'a> for &'a bool { |
There was a problem hiding this comment.
all these impls should be abstracted away into a macro
There was a problem hiding this comment.
I was thinking about it too lately. I'm not sure how to define the macro to be readable enough. I was thinking about something like this:
impl_try_borrows!( Bool => bool, I64 => i64, /* etc. */ );
What do you think?
| @@ -473,6 +559,99 @@ impl de::Deserialize for Value { | |||
| } | |||
| } | |||
|
|
|||
| /// Represents type of a JSON value | |||
| #[derive(Clone, Copy, Eq, PartialEq, Debug)] | |||
| pub enum ValueType { | |||
There was a problem hiding this comment.
This type sounds a lot like std::mem::discriminant_value, so we could just wait until that is stable (3 Months probably)
There was a problem hiding this comment.
I agree. The library is still unstable, so we can wait. I hope time is not concern to anyone else.
I think I'm going to leave this as is for now until discriminant is stabilized. However, I'm not sure whether discriminant will allow to do exactly what I mean. I will check that out.
|
The motivation is mostly to let inference figure out the type. It would allow users to assign the resulting borrow into struct or passing it to function without thinking about types. Also, if they later decide to change the type, it will adapt automatically. I'm not sure whether this will lead to unreadable messages. Do you have an example of such message? I also noticed issue #126 (see also #127). This PR solves the naming problem quite elegantly. Instead of specifying the name in function (and deciding which is the best), it allows user to specify the exact type. At least for I believe something similar could be implemented for Finally, please note that I intentionally used custom error type, so it can be displayed to user in very human friendly way. ("type mismatch: expected type: X found type: Y") I hope I made myself clear enough. I'm looking forward for your thoughts! |
|
@oli-obk what is the status of this PR? Is it waiting for stable |
Just less code for the same effect
Imagine calling the method on a Will review more later |
|
Reading through this again, I don't think this would be a beneficial change. It seems like the three main motivations for this PR are:
|
|
This PR was posted a long time ago and serde changed quite a lot since then. I'd have to look at it again and re-validate. I'm fine with closing now. |
This change adds as_borrow() generic method for Value. It allows users to use e.g.
some_fn(try!(value.as_borrow()))instead ofsome_fn(try!(value.as_string().ok_or(SomeCustomError)))Ability to borrow mutably may be even more interesting.
It also defines nice error type for situations when types don't match as well as representation of value type. Those things create dependency chain.