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

add some temporal methods #3856

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open

add some temporal methods #3856

wants to merge 19 commits into from

Conversation

jasonwilliams
Copy link
Member

@jasonwilliams jasonwilliams commented May 15, 2024 •
edited
Loading

Adds methods to yearMonth and monthDay

Relies on boa-dev/temporal#44

@jasonwilliams jasonwilliams added the enhancement New feature or request label May 15, 2024
@github-actions GitHub Actions
Copy link

Test262 conformance changes

Test result main count PR count difference
Total 50,731 50,731 0
Passed 42,973 42,980 +7
Ignored 1,395 1,395 0
Failed 6,363 6,356 -7
Panics 18 18 0
Conformance 84.71% 84.72% +0.01%
Fixed tests (7): 
test/built-ins/Temporal/PlainYearMonth/calendar-undefined.js (previously Failed)
test/built-ins/Temporal/PlainYearMonth/prototype/calendarId/branding.js (previously Failed)
test/built-ins/Temporal/PlainYearMonth/prototype/calendarId/builtin-calendar-no-observable-calls.js (previously Failed)
test/built-ins/Temporal/PlainYearMonth/prototype/month/branding.js (previously Failed)
test/built-ins/Temporal/PlainYearMonth/prototype/month/builtin-calendar-no-observable-calls.js (previously Failed)
test/built-ins/Temporal/PlainYearMonth/prototype/year/branding.js (previously Failed)
test/built-ins/Temporal/PlainYearMonth/prototype/year/builtin-calendar-no-observable-calls.js (previously Failed)

nekevss
nekevss reviewed May 16, 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.

Just a little early feedback 😄

core/engine/src/builtins/temporal/plain_year_month/mod.rs Outdated Show resolved Hide resolved
@jasonwilliams
Copy link
Member Author

linked to boa-dev/temporal#44

@jasonwilliams jasonwilliams marked this pull request as ready for review May 24, 2024 16:11
nekevss
nekevss reviewed Jul 30, 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.

PartialDuration update looks good to me! Had a couple more comments around MonthDay. 😄

core/engine/src/builtins/temporal/duration/mod.rs
}

// 12. Let minutes be ? Get(temporalDurationLike, "minutes").
let minutes = unknown_object.get(js_str!("minutes"), context)?;
// 13. If minutes is not undefined, set result.[[Minutes]] to ? ToIntegerIfIntegral(minutes).
if !minutes.is_undefined() {
result.set_minutes(f64::from(to_integer_if_integral(&minutes, context)?));
result.minutes = Some(FiniteF64::from(to_integer_if_integral(&minutes, context)?));
Copy link
Member

Choose a reason for hiding this comment

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

thought: It might be nice if we had a JsValue::map that acted similar to Option::map, where Undefined is None.

core/engine/src/builtins/temporal/duration/mod.rs

// 3. NOTE: The following steps read properties and perform independent validation in alphabetical order.
// 4. Let days be ? Get(temporalDurationLike, "days").
let days = unknown_object.get(js_str!("days"), context)?;
if !days.is_undefined() {
// 5. If days is not undefined, set result.[[Days]] to ? ToIntegerIfIntegral(days).
result.set_days(f64::from(to_integer_if_integral(&days, context)?));
result.days = Some(FiniteF64::from(to_integer_if_integral(&days, 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: potentially use in-place Option operations.

There may be some benefit to use either insert or get_or_insert over " = Some(...) ". i.e. result.days.insert(FiniteF64::from(...));

core/engine/src/builtins/temporal/plain_month_day/mod.rs
fn from(_: &JsValue, args: &[JsValue], context: &mut Context) -> JsResult<JsValue> {
let options = get_options_object(args.get_or_undefined(1))?;
let item = args.get_or_undefined(0);
let inner = if item.is_object() {
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: extract into to_temporal_month_day function

Technically, ToTemporalMonthDay is only called twice by by from and equals, so I could see the argument to not extract it. But having a to-X method implementation would be consistent with all the other built-ins and hopefully make everything a bit more readable.

core/engine/src/builtins/temporal/duration/mod.rs
} else {
temporal_duration_like.years()
};
let years = temporal_duration_like
Copy link
Member

Choose a reason for hiding this comment

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

thought: I definitely much prefer this to the previous code!

@nekevss
Copy link
Member

nekevss commented Jul 31, 2024

Was double checking this. I think the new revision of temporal_rs needs to be added, and then I think this PR should be just about good to go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jedel1043 jedel1043 jedel1043 left review comments

@nekevss nekevss nekevss left review comments

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jasonwilliams @nekevss @jedel1043