RUST-977 Support RFC 3339 to bson::DateTime conversion without chrono feature flag
#317
Conversation
src/datetime.rs
Outdated
| } | ||
|
|
||
| /// Convert the given RFC 3339 format string to a [`DateTime`]. | ||
| pub fn from_rfc3339(s: &str) -> Result<Self, crate::de::Error> { |
There was a problem hiding this comment.
This can be made slightly more convenient for the caller by changing the type of the parameter to impl AsRef<str> (and then using it via s.as_ref() in the line below) - that'll let it accept both &str and String values.
src/datetime.rs
Outdated
| pub fn from_rfc3339(s: &str) -> Result<Self, crate::de::Error> { | ||
| match chrono::DateTime::<chrono::FixedOffset>::parse_from_rfc3339(s) { | ||
| Ok(d) => Ok(Self::from_chrono(d)), | ||
| Err(_) => Err(crate::de::Error::InvalidTimestamp { |
There was a problem hiding this comment.
This should format the error returned from parse_from_rfc3339 either instead of or as part ofmessage.
There was a problem hiding this comment.
Changed to returning the error message from parse_from_rfc3339 in 7bcb901.
| crate::DateTime::from_rfc3339(rfc).unwrap(), | ||
| crate::DateTime::from_chrono(date) | ||
| ); | ||
| } |
There was a problem hiding this comment.
I think it'd be good to also have a test to validate failure behavior - doesn't need to assert on the exact error message, just that one is returned for invalid input.
There was a problem hiding this comment.
Added invalid test cases in 7bcb901.
src/de/error.rs
Outdated
| InvalidUtf8String(string::FromUtf8Error), | ||
|
|
||
| /// An error encountered during the conversion of one datetime format to another. | ||
| InvalidTimestamp { |
There was a problem hiding this comment.
rather than reusing the deserialization error here, I wonder if it would be better to introduce another error type for datetimes like we have for ObjectIds (oid::Error, returned from ObjectId::parse_str) and UUIDs (uuid::Error, returned from Uuid::parse_str). It's unfortunate that this crate has so many error types, but I think it's something we probably have to commit to at this point.
There was a problem hiding this comment.
Yeah that seems fine to me for the sake of consistency
There was a problem hiding this comment.
Added an error enum and switched over to using that instead in 0bd1b00.
src/de/error.rs
Outdated
| InvalidUtf8String(string::FromUtf8Error), | ||
|
|
||
| /// An error encountered during the conversion of one datetime format to another. | ||
| InvalidTimestamp { |
There was a problem hiding this comment.
Yeah that seems fine to me for the sake of consistency
src/tests/datetime.rs
Outdated
| assert_eq!( | ||
| crate::DateTime::parse_rfc3339_str(rfc).unwrap(), | ||
| crate::DateTime::from_chrono(date) | ||
| ); |
There was a problem hiding this comment.
can we add an assertion that converting the datetime back to a string matches the input?
| /// Errors that can occur during [`DateTime`] construction and generation. | ||
| #[derive(Clone, Debug)] | ||
| #[non_exhaustive] | ||
| pub enum Error { |
There was a problem hiding this comment.
To allow users to refer to this type, we'll need to make the datetime module pub in lib.rs. Then users can say bson::datetime::Result.
RUST-977
Add a method to convert a RFC 3339 formatting string to
bson::DateTimeeven without the chrono feature flag.