Temporal parser #379
Temporal parser #379
Conversation
There was a problem hiding this comment.
Hey! Thanks for working on this! I left a few review comments. Overall this is looking pretty good!
In general, the gist of the feedback is that the parser should ideally take utf8 and utf16 as primaries (a from_str method is an option assuming it calls str::to_bytes internally using the utf8 method).
For now, this can focus on primarily supporting utf8, but once ixdtf is bumped to its current main or a new version, the utf16 support will need to be added.
| impl TemporalParser { | ||
| /// Creates a new `TemporalParser`. | ||
| #[inline] | ||
| pub const fn new() -> Self { |
There was a problem hiding this comment.
issue: the constructor in this case should be fairly similar to IxdtfParser, using from_utf8 and from_utf16.
In general, this parser should probably wrap IxdtfParser or the source for IxdtfParser. Now that I think about it, the best approach is probably going to be storing the source. The IxdtfParser does not reset after a parse, which could probably be updated up stream. For now, this could be done with from_utf8 and from_utf16 can be added in later with the ixdtf bump.
| @@ -804,11 +1219,17 @@ pub(crate) fn parse_time(source: &[u8]) -> TemporalResult<TimeRecord> { | |||
| Ok(time) => { | |||
| if time.offset == Some(UtcOffsetRecordOrZ::Z) { | |||
| return Err(TemporalError::range() | |||
| .with_message("UTC designator is not valid for DateTime parsing.")); | |||
| .with_message("UTC designator is not valid for Time parsing.")); | |||
| /// The validated ISO DateTime components | ||
| pub iso: IsoDateTime, | ||
| /// Optional calendar identifier as a string | ||
| pub calendar: Option<String>, |
There was a problem hiding this comment.
issue: this should either be &[u8]/&[u16] or Calendar
Ideally, this shouldn't allocate a String as an intermediate. In an upcoming release of ixdtf, there will be handling for both utf8 and utf16, so it would probably be fine to have either &[u8] or Calendar for now.
nekevss
added
C-enhancement
|
Quick follow up, the |
Addresses #224