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.
nekevss
added
C-enhancement
|
Quick follow up, the |
|
Hi @blarfoon, are you able to fix the above? |
blarfoon
force-pushed
the
temporal-parser
branch
from
36976ea to
f161a21
Compare
| } | ||
|
|
||
| /// Converts a UTF-16 encoded string into a `PlainDate`. | ||
| pub fn from_utf16(s: &[u16]) -> TemporalResult<Self> { |
There was a problem hiding this comment.
n.b. temporal_capi has a bunch of internally-converting from_utf16s that probably can be swapped over to these
| } | ||
|
|
||
| /// Parses the source into a `PlainDateTime` compatible record. | ||
| pub fn parse_date_time(&self) -> TemporalResult<ParsedDateTime<'a>> { |
There was a problem hiding this comment.
suggestion: ideally we link to exactly which part of the spec this implements: there are a lot of parseable things called DateTime
| ) -> TemporalResult<IxdtfParseRecord<'a, Utf16>> { | ||
| let record = self.parse_ixdtf_utf16(source, ParseVariant::DateTime)?; | ||
|
|
||
| if record.offset == Some(UtcOffsetRecordOrZ::Z) { |
There was a problem hiding this comment.
issue: I think this code should be shared; same for the rest.
| } | ||
| } | ||
|
|
||
| fn parse_ixdtf_utf16( |
There was a problem hiding this comment.
observation: I also think this code should be shareable.
I think all of this can be made to operate on an IxdtfParser, and the end functions construct an IxdtfParser::from_whatever and pass it down.
Hell, I think the IxdtfParser can probably just be a field in TemporalParser.
|
|
||
| fn parse_date_time_utf16( | ||
| &self, | ||
| source: &'a [u16], |
There was a problem hiding this comment.
question: what's the point of taking both a source and self here? Both wrap the same string?
| Self::DateTimeOutOfRange => { | ||
| TemporalError::range().with_message("Date/time is outside representable range") | ||
| } | ||
| Self::ParseError(msg) => TemporalError::syntax().with_message(msg), |
There was a problem hiding this comment.
nit: this should probably be a RangeError
While it would be technically correct for this to be a syntax error, almost all syntax errors are thrown as range errors, so the general movement has been away from using them.
|
@blarfoon do you still plan to work on this? This PR may have to be closed and rewritten since it got very outdated with the current repo. |
|
Closing this PR since it got stale. |
4 participants
Addresses #224