-
-
Notifications
You must be signed in to change notification settings - Fork 401
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
update Boa to be inline with Temporal #4034
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4034 +/- ##
==========================================
+ Coverage 47.24% 52.85% +5.61%
==========================================
Files 476 483 +7
Lines 46892 46980 +88
==========================================
+ Hits 22154 24832 +2678
+ Misses 24738 22148 -2590 ☔ View full report in Codecov by Sentry. |
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.
Overall looks good to me! I have a couple of nits that I would prefer are addressed, but nothing that is necessarily blocking.
I think there were a couple more updates in the temporal_rs
mainly around ZonedDateTime
. Were these still in the work?
030131c
to
57b8c46
Compare
@nekevss i've cleaned up the comments and rebased, ill look at zonedDateTime in a follow up PR |
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.
Just a small suggestion. Everything else looks good!
- md_record from parse_ixdtf is enough, we shouldn't need to fall back to datetime parsing. - from_str should lowercase the iso8601 (fixes tests) - Support parsing the ISO string then getting the calendar from that. - Support ref year This is to support changes in boa-dev/boa#4034
@@ -957,3 +988,21 @@ pub(crate) fn to_partial_date_record( | |||
era_year, | |||
}) | |||
} | |||
|
|||
impl From<Option<u16>> for JsValue { |
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.
I was just working on integrating ZonedDateTime
with the branch I've been working on. And when updating Boa, likewise to this PR I realized that this could just use IntoOrUndefined
to convert the Option<T>
into a JsValue
.
This PR brings Boa inline with the latest changes in Temporal