-
Notifications
You must be signed in to change notification settings - Fork 19
Updates to instant and its methods #85
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
src/components/instant.rs
Outdated
// `temporal_unwraps` MUST be present. | ||
|
||
// Find the IsoDate | ||
let date = ixdtf_record.date.temporal_unwrap()?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, is this asserting an invariant? Because temporal_unwrap
should only be used to ensure invariants are preserved. If this is used to unwrap errors caused by the user, then I think it should return an error instead, which is a bit more explanatory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be an invariant at this point. parse_instant
requires a string with a DateRecord
, TimeRecord
, and OffsetRecord
, and throws an error if any of them are None
, which is what the NOTE was trying to highlight. 😆 Although reading it back now, the note probably makes it more ambiguous.
We can return an error in these places again, but they would be redundant errors that realistically would never throw.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, now I see why. On parse_instant
, we validate that the fields are Some
and throw otherwise. Can we move that invariant to the typesystem? Instead of returning an IxdtfParseRecord
, we return IxdtfParseInstantRecord
with the validated fields being not Option
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
This PR updates Instant to some of the newer specification changes.
TLDR of the below:
Instant
structFromStr
.Primarily, it brings
since
anduntil
up to date with bothtemporal_rs
and the specification as a whole (along with at least 2 basic tests forsince
anduntil
)This PR also changes Instant inner nanoseconds from
BigInt
toi128
. My thought is to eventually remove all usage ofBigInt
fromtemporal_rs
in general. Primarily motivated by the thought that ifNormalizedTimeDuration
is using ai128
, I don't see a huge reason whyInstant
should differ, unless I'm overlooking something and someone thinks otherwise.Finally, it implements
FromStr
forInstant
. This will allow parsing ofJsString
in Boa, and would unblockto_temporal_instant
.