Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Swap to Duration::round from temporal_rs #3731

Merged
merged 6 commits into from
Mar 25, 2024
Merged

Swap to Duration::round from temporal_rs #3731

merged 6 commits into from
Mar 25, 2024

Conversation

robot-head
Copy link
Contributor

This Pull Request fixes part of #{11}.

It changes the following:

  • Swaps Duration::round implementation to temporal_rs's

@codecov Codecov
Copy link

codecov bot commented Mar 10, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 0% with 14 lines in your changes are missing coverage. Please review.

Project coverage is 47.66%. Comparing base (6ddc2b4) to head (27b4dce).
Report is 98 commits behind head on main.

Files Patch % Lines
core/engine/src/builtins/temporal/mod.rs 0.00% 7 Missing ⚠️
core/engine/src/builtins/temporal/options.rs 0.00% 4 Missing ⚠️
core/engine/src/builtins/temporal/duration/mod.rs 0.00% 3 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3731      +/-   ##
==========================================
+ Coverage   47.24%   47.66%   +0.42%     
==========================================
  Files         476      454      -22     
  Lines       46892    44568    -2324     
==========================================
- Hits        22154    21245     -909     
+ Misses      24738    23323    -1415

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@robot-head
Copy link
Contributor Author

Paired with boa-dev/temporal#34 gets Duration.round conformance up to 27.93%

Suite test262\test/built-ins/Temporal/Duration/prototype/round results: total: 111, passed: 31, ignored: 0, failed: 80 , conformance: 27.93%


Results (ECMAScript Next):
Total tests: 111
Passed tests: 31
Ignored tests: 0
Failed tests: 80 (0 panics)
Conformance: 27.93%

nekevss
nekevss requested changes Mar 11, 2024
View reviewed changes
Copy link
Member

@nekevss nekevss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work on this overall!

Had a couple suggestions and a nitpick 😄

core/engine/src/builtins/temporal/mod.rs Outdated
context: &mut Context,
) -> RelativeTemporalObjectResult {
let relative_to = options.get(PropertyKey::from(utf16!("relativeTo")), context)?;
let plane_date = match relative_to {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: plain_date over plane_date

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops! lol

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

core/engine/src/builtins/temporal/duration/mod.rs Outdated
let _rounding_mode = get_option(&round_to, utf16!("roundingMode"), context)?
.unwrap_or(RoundingMode::HalfExpand);
let rounding_mode: Option<TemporalRoundingMode> =
get_option::<RoundingMode>(&round_to, utf16!("roundingMode"), context)?.map(Into::into);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: you should be able to use get_option::<TemporalRoundingMode> here as ParsableOptionType is implemented for TemporalRoundingMode

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

core/engine/src/builtins/temporal/duration/mod.rs Outdated
Err(JsNativeError::range()
.with_message("not yet implemented.")
.into())
let duration = to_temporal_duration(this, context)?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: this along with to_temporal_duration call can be replaced with the duration on line 605

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@nekevss nekevss requested a review from a team March 11, 2024 01:31
@nekevss nekevss added the builtins PRs and Issues related to builtins/intrinsics label Mar 11, 2024
@jedel1043 jedel1043 requested a review from nekevss March 24, 2024 05:43
@jedel1043 jedel1043 added the waiting-on-review Waiting on reviews from the maintainers label Mar 24, 2024
@nekevss nekevss requested a review from a team March 25, 2024 00:10
@jedel1043 jedel1043 added this pull request to the merge queue Mar 25, 2024
Merged via the queue into boa-dev:main with commit 48750f3 Mar 25, 2024
14 checks passed
@jedel1043 jedel1043 removed the waiting-on-review Waiting on reviews from the maintainers label Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nekevss nekevss nekevss approved these changes

@jedel1043 jedel1043 jedel1043 approved these changes

Assignees
No one assigned
Labels
builtins PRs and Issues related to builtins/intrinsics
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@robot-head @jedel1043 @nekevss