Implementation of toZonedDateTimeISO for Instant#258
Conversation
|
Huh.. strange that one of the tests fail here.. Passes locally, and that file is not touched by this PR |
|
Huh, are you running on an ubuntu distro? Also, have you rebased to main for this PR? |
I think that should be consistent with some of the other methods then. There are some parsing bugs for time zones that should be fixed either when we update |
src/builtins/core/timezone.rs
Outdated
|
|
||
| pub fn try_from_str(src: &str) -> TemporalResult<Self> { | ||
| if let Ok(timezone) = Self::try_from_identifier_str(src) { | ||
| let normalized = src.to_uppercase(); |
There was a problem hiding this comment.
Oh, wait this is probably what's causing the failure.
So this is actually sort of a tricky case.
If you upper case a time zone string, "America/New_York" becomes "AMERICA/NEW_YORK". The file system will return no such file or directory because it's case sensitive (although, this may actually depend on your OS configuration). That's actually what #251 is working towards, providing a case insensitive way to return the normalized time zone strings (recommend taking a look at debug/iana_normalizer.json).
lockels
force-pushed
the
instant_to_zdt_iso
branch
from
75577d7 to
2fc0cac
Compare
Nope, makes sense why it fails since i'm on a mac.
Nope, but i think i managed to fix that now. I'll convert this PR to a draft for now.. |
lockels
force-pushed
the
instant_to_zdt_iso
branch
from
6bbd835 to
2e3e513
Compare
src/builtins/core/instant.rs
Outdated
| // TODO: May end up needing a provider API during impl | ||
| pub fn to_zoned_date_time_iso(&self, _time_zone: TimeZone) -> TemporalResult<ZonedDateTime> { | ||
| Err(TemporalError::general("Not yet implemented")) | ||
| pub fn to_zoned_date_time_iso(&self, time_zone: TimeZone) -> TemporalResult<ZonedDateTime> { |
There was a problem hiding this comment.
thought: I think this can be made non-fallible.
Calendar::from_utf8(b"iso8601") can be Calendar::default(), and then this method returns ZonedDateTime directly without a result.
This is inline with the specification:
8.3.15 Temporal.Instant.prototype.toZonedDateTimeISO ( timeZone )
This method performs the following steps when called:
1. Let instant be the this value.
2. Perform ? RequireInternalSlot(instant, [[InitializedTemporalInstant]]).
3. Set timeZone to ? ToTemporalTimeZoneIdentifier(timeZone).
4. Return ! CreateTemporalZonedDateTime(instant.[[EpochNanoseconds]], timeZone, "iso8601").
Note how the major step we are handling here is step 4, which is non-fallible. The fallible portion is in step 2 (engine specific step) and step 3 (handled by the conversion to TimeZone).
lockels
force-pushed
the
instant_to_zdt_iso
branch
from
07984e3 to
ccdfab4
Compare
nekevss
left a comment
nekevss
left a comment
There was a problem hiding this comment.
Looks great! Thanks as always!
2 participants
Fixes #246
Implementation of toZonedDateTimeISO for Instant.
most tests pass, one strange test262 function looks like it should pass, but doesn't somehow.
https://github.com/tc39/test262/blob/main/test/built-ins/Temporal/Instant/prototype/toZonedDateTimeISO/timezone-string-datetime.js
This one in particular.. The code does indeed throw a range error when trying to create a timezone from those strings.. but the test doesn't pass.