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 7 commits into
base: main
Choose a base branch
from
Open

add some temporal methods #3856

wants to merge 7 commits into from

Conversation

jasonwilliams
Copy link
Member

@jasonwilliams jasonwilliams commented May 15, 2024 •
edited

Adds methods to yearMonth

month
year
calendarId
monthCode
daysInYear
daysInMonth
monthsInYear
inLeapYear

@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 Jun 2, 2024
View reviewed changes
core/engine/src/builtins/temporal/plain_year_month/mod.rs
.expect("Error adding duration to year month");
create_temporal_year_month(year_month_result, None, context)
} else {
return Err(JsNativeError::typ()
Copy link
Member

Choose a reason for hiding this comment

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

There is a FromStr implementation for Duration. Was the implementation not working in this scenario?

core/engine/src/builtins/temporal/plain_year_month/mod.rs
fn from(_: &JsValue, args: &[JsValue], context: &mut Context) -> JsResult<JsValue> {
let item = args.get_or_undefined(0);
// 1. If Type(item) is Object or Type(item) is String and item is not null, then
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.

question: Is this up to date?

I currently have the specification showing the below for 9.2.2, which may actually simplify the logic here if so.

2. If item is an Object and item has an [[InitializedTemporalYearMonth]] internal slot, then

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

@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
2 participants
@jasonwilliams @nekevss