initial implementation of ToZonedDateTime, ToPlainDate, ToPlainTime#244
initial implementation of ToZonedDateTime, ToPlainDate, ToPlainTime#244nekevss merged 5 commits intoboa-dev:mainfrom
Conversation
nekevss
left a comment
nekevss
left a comment
There was a problem hiding this comment.
Looks great!
This is about ready to merge, but I do have a couple nits that would be nice to have addressed. Mostly because they could be easily forgotten if not addressed in this PR and it's within scope.
| Ok(Self::new_unchecked(result, self.calendar.clone())) | ||
| } | ||
|
|
||
| pub fn to_zoned_date_time_with_provider( |
There was a problem hiding this comment.
nit: add a PlainDateTime::to_zoned_date_time implementation on PlainDateTime in compiled.
It looks like I missed adding this stub. So a new module should be added to the compiled directory for PlainDateTime and the companion method added.
Worthwhile note: there's no current need to add this method to temporal_capi just yet because time zone providers over the FFI is still a work in progress. So only the non provider method implementation is needed.
| )) | ||
| } | ||
|
|
||
| pub fn to_plain_date(&self) -> TemporalResult<PlainDate> { |
There was a problem hiding this comment.
nit: add a temporal_capi implementation of to_plain_date for the FFI
This one may be a little harder because it's dealing with the diplomat bindings, but maybe you can take a crack at it. Changes should be made here.
lockels
force-pushed
the
plain_date_time
branch
from
857f25d to
b80d6f2
Compare
|
Broken CI is from the new FFI needing to be checked in. Just run: Commit the generated output, and everything should be good to go. |
nekevss
left a comment
nekevss
left a comment
There was a problem hiding this comment.
Looks great! Thanks again!
As a heads up, I'm going to open up an issue partially related to this PR about return types used, but I think it's a broader issue across the repository and out of scope of this specific PR.
2 participants
Fixes #149