-
-
Notifications
You must be signed in to change notification settings - Fork 407
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
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4034 +/- ##
==========================================
+ Coverage 47.24% 53.14% +5.90%
==========================================
Files 476 484 +8
Lines 46892 47735 +843
==========================================
+ Hits 22154 25371 +3217
+ Misses 24738 22364 -2374 ☔ 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
7d68c3c
to
b01bf93
Compare
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! I have a couple of suggestions.
let day = item | ||
.get_v(js_string!("day"), context) | ||
.expect("Day not found") | ||
.to_i32(context) | ||
.expect("Cannot convert day to i32"); | ||
let month = item | ||
.get_v(js_string!("month"), context) | ||
.expect("Month not found") | ||
.to_i32(context) | ||
.expect("Cannot convert month to i32"); | ||
|
||
let month_code = item | ||
.get_v(js_string!("monthCode"), context) | ||
.expect("monthCode not found"); | ||
let resolved_month_code = if month_code.is_undefined() { | ||
None | ||
} else { | ||
TinyAsciiStr::<4>::from_str( | ||
&month_code | ||
.to_string(context) | ||
.expect("Cannot convert monthCode to string") | ||
.to_std_string_escaped(), | ||
) | ||
.map_err(|e| JsError::from(JsNativeError::range().with_message(e.to_string()))) | ||
.ok() | ||
}; | ||
let year = item.get_v(js_string!("year"), context).map_or(1972, |val| { | ||
val.to_i32(context).expect("Cannot convert year to i32") | ||
}); |
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.
Thought: this was already here, but I'm a bit worried that we're just expect
ing a lot of missing values instead of throwing errors.
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.
😕 Hmmmm, that's a good point.
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 good!
This PR brings Boa inline with the latest changes in Temporal