Add custom Duration and Timestamp types for conversion with serde #89
Conversation
fore5fire
force-pushed
the
serde-datetime
branch
from
0c082ac
to
155548e
Compare
|
I did some research and found the clever idea to use serde's newtype_struct name field as the indicator to do special type handling. Since serde's official recommendation is to silently unwrap newtypes, it's a great way to signal to a custom Serializer but remain compatible with most other implementations |
fore5fire
force-pushed
the
serde-datetime
branch
from
28b67cb
to
62ea6f2
Compare
fore5fire
force-pushed
the
serde-datetime
branch
from
1222943
to
18ddbb6
Compare
|
Updated for the new |
| fn serialize_struct(self, name: &'static str, len: usize) -> Result<Self::SerializeStruct> { | ||
| assert!(matches!(self, Self::Duration { .. })); | ||
| assert_eq!(name, "Duration"); | ||
| assert_eq!(len, 2); | ||
| Ok(SerializeTimestamp::default()) | ||
| } | ||
|
|
||
| fn serialize_str(self, v: &str) -> Result<Value> { | ||
| assert!(matches!(self, Self::Timestamp)); | ||
| Ok(v.parse::<chrono::DateTime<FixedOffset>>() | ||
| .map_err(|e| SerializationError::SerdeError(e.to_string()))? | ||
| .into()) | ||
| } |
There was a problem hiding this comment.
Why are we using asserts here instead of returning an error result? This would be fine for application code, but for a library, I don't want to prevent users from being able to deal with bad inputs. If these are considered impossible conditions, please add a SAFETY: comment indicating the invariants that will be upheld and how a reader can prove that these panics will never occur.
Currently
chrono::Durationdoesn't implementSeralizeandchrono::DateTimeserializes to an ISO 8601 string. This change is inspired by the toml crate's approach to preserving date formats across functions available in theSerializetrait to ensure that durations and timestamp are converted into the appropriate CEL types.Using the newly exposed
DurationandTimestamptypes is only necessary for types usingserdeto convert to CEL, but maybe it could be confusing to keep track of where to use thechronotypes vs thecel_interpretertypes? But it seems like an unnecessary breaking change to use the new types inValue::DurationandValue::Timestamp🤷🏼