Enhance holochain_core_types::time::Iso8601 with DateTime conversions #917
Conversation
…ature-Iso8601-conversions
|
ok cool, the string type for iso8601 was only ever intended as a quick placeholder until something more comprehensive can be done |
o Timezones offsets must be full +/-HH:MM to be accepted by RFC 3339 o Provide a fmt::Debug that displays ISO 8601 conversions, and an error for invalid timestamp. o To support sortability, the Cmp and Eq total-order traits are provided which deem all invalid Iso8601 timestamps to be equivalent, and less than any valid Iso8601 for sorting purposes.
yes, invalid data is always an invalid call/result
dang... but that sounds like an important-to-fix-bug in the tests to me :(
yes please!
returning an error result from a
i'm pretty sure @lucksus is saying that the read over the "runtime impossible" docs that i wrote up for more info: https://hackmd.io/QPgBfOk4TwOMrZnCCE46ZA?both#Runtime-impossible
feels like kicking the can down the road the longer we leave a correct implementation, the more work/LOC it will be ultimately as we build more infra on top of it fixing this RN is the easiest/fastest it will ever be IMO |
|
Thanks, @thedavidmeister -- I agree with everything you said. I'll work toward that. The failing serialization on Zome API calls needs some serious, serious work -- basically (last time I checked), it was an opaque, valueless error, giving no indication of what you did wrong. Sort of like filling out pages of paperwork, and getting it back with a "rejected" stamp, with no indication of what you did wrong... :) |
yerp |
o Only Iso8601::from UNIX timestamp is infallible o Standard Display/Debug fmt and Serialize/Deserialize string format is ISO 8601 / RFC 3339. o Remove unnecessary println! in RibosomeErrorCode, and replace some unhelpful unreachable() panics w/ an indication of what failed.
|
Ok, @lucksus, @thedavidmeister, @zippy -- at long last, I think this Iso8601 might be ready to look at again. There wasn't many Rust unit tests that failed; I was certain that previously there were more tests that used Iso8601::from( &str ), but maybe I was mistaken. Anyway, all the Rust unit tests pass cleanly. There are a few other changes, mostly around returning errors that help diagnose Zome API call failures. |
| } | ||
|
|
||
| /* | ||
| * Note that the WASM target does not have a reliable and consistent means to obtain the local time, |
There was a problem hiding this comment.
This is important. @Connoropolous take note for documentation!!
|
|
||
| fn from_str(s: &str) -> Result<Self, Self::Err> { | ||
| lazy_static! { | ||
| static ref ISO8601_RE: Regex = Regex::new( |
There was a problem hiding this comment.
Yowza! That's one hecka regular expression!
| //! The Iso8601 type is defined here. It is used in particular | ||
| //! within ChainHeader to enforce that their timestamps | ||
| //! are defined in a useful and consistent way. | ||
| //! The Iso8601 type is defined here. It is used in particular within ChainHeader to enforce that |
There was a problem hiding this comment.
This is a superb file @pjkundert . I think really we should be abstracting this out as its own rust crate for contribution to the community, and then using it. That would also increase surface area for removing any bugs in it, if others are using it for orthogonal purposes.
There was a problem hiding this comment.
Cool, this looks very solid! @thedavidmeister, I think we can merge this now, right?
|
Haven't heard from @thedavidmeister so I'm merging. We can fix thing later... |
|
great result, soz for the delayed response |
The Iso8601 type is currently a simple wrapper around String, with no checking. For Holofuel, we need to be able to actually operate on these as DateTime timestamps. Handling such conversions and comparisons gracefully is non-trivial.
I have enhanced Iso8601 to maintain un-checked From traits, and have added String, in addition to static &str. This supports importation of Iso8601 data from various external sources.
I have added checked TryFrom for &Iso8601 -> DateTime, which supports direct RFC 3339 conversion, as well as supports a greater subset of ISO 8601 timestamps, at the expense of a Regex invocation. So, you pay for what you get; quick conversion for straight, simple RFC 3339 timestamps, and support for richer ISO 8601 from external sources. Particularly, it defaults timestamps w/ no Time Zone specification to UTC, or "Zulu" time.
This validation is done at initial
Iso8601::newtime, and stored for future use for fmt::Display, PartialOrd, etc. Serialization and fmt::Display always displays the canonicalized ISO 8601 form of the original timestamp (except if invalid; then the original String is produced).Finally, the PartialOrd and PartialEq have been implemented. Since not all Iso8601 are comparable (because they may be invalid), we treat the comparison of invalids for Equality as always false, and present None for partial_cmp (somewhat like float NaN are treated).