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 Temporal.Calendar.prototype.daysIn* tests. #3052

Merged
merged 6 commits into from
Aug 6, 2021
Merged

Add Temporal.Calendar.prototype.daysIn* tests. #3052

merged 6 commits into from
Aug 6, 2021

Conversation

FrankYFTang
Copy link
Contributor

rwaldron
rwaldron requested changes Jul 19, 2021
View reviewed changes
Copy link
Contributor

@rwaldron rwaldron left a comment

Choose a reason for hiding this comment

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

Please fix the directory names.

@FrankYFTang FrankYFTang requested a review from rwaldron July 19, 2021 18:24
@FrankYFTang
Copy link
Contributor Author

PTAL

@rwaldron
Copy link
Contributor

@FrankYFTang how are you testing these against test262-harness?

rwaldron
rwaldron reviewed Jul 19, 2021
View reviewed changes
test/built-ins/Temporal/Calendar/prototype/daysInMonth/plain-date.js Outdated
esid: sec-temporal.calendar.prototype.daysinmonth
description: Temporal.Calendar.prototype.daysInMonth will take Temporal.PlainDate object
and return the number of days in that month.
and return Array of the same content.
Copy link
Contributor

Choose a reason for hiding this comment

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

Which one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry. Bad copy and paste.

@FrankYFTang FrankYFTang requested a review from rwaldron July 19, 2021 21:30
@FrankYFTang
Copy link
Contributor Author

PTAL

@FrankYFTang
Copy link
Contributor Author

@FrankYFTang how are you testing these against test262-harness?

Sorry, I do not understand your question. I prototype an implemention in v8 and code the test262 tests in v8 tree, run it and see it's outcome is reasonable or not, then I copy over the file to test262 tree and upload the PR. In my v8 build I ran

$ tools/run-tests.py --outdir=out/x64.release test262/built-ins/Temporal/Calendar*

Does this answer your question?

@rwaldron
Copy link
Contributor

I prototype an implemention in v8

This is the information I was looking for. When will any of that be available in nightlies?

@FrankYFTang
Copy link
Contributor Author

I prototype an implemention in v8

This is the information I was looking for. When will any of that be available in nightlies?

I am aiming to land a rough prototype behind the flag by end of Q3 with full internationalization support.
See https://docs.google.com/document/d/1Huu2OUlmveBh4wjgx0D7ouC9O9vSdiZWaRK3OwkQZU0/edit
I am single hand on this project now and it is not easy to wire up ~300 public functions + 200+ AOs....

This is my main task for Q3 and so far so good.
Prototype (not landing) progress tracking https://docs.google.com/spreadsheets/d/1f-S70nmYOMbp_8aF4GE7uDCZ3caLoFnsL4-DatqMfSg/edit#gid=0

very rough prototype cl (not ready for review) in https://chromium-review.googlesource.com/c/v8/v8/+/2967755 in case you like to patch v8 and play with it. Not advise you to do so. Better wait another 2.5 months after I land most of the functionality.

@anba
Copy link
Contributor

anba commented Jul 22, 2021

I can also upload my patches for https://bugzilla.mozilla.org/show_bug.cgi?id=1519167, if that is useful for anyone. I didn't yet upload them because there are still many open issues (tc39/proposal-temporal#1502 covers roughly half of my notes about spec issues), so I was waiting for the proposal to be a bit more polished.

@FrankYFTang FrankYFTang requested a review from rwaldron August 5, 2021 22:59
@FrankYFTang
Copy link
Contributor Author

PTAL
correct the expect/actual order. Add more tests to cover the ? AO

@rwaldron rwaldron mentioned this pull request Aug 6, 2021
Closed
@rwaldron rwaldron merged commit 86e2399 into tc39:main Aug 6, 2021
@FrankYFTang FrankYFTang deleted the daysIn branch August 9, 2021 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ptomato ptomato ptomato left review comments

@rwaldron rwaldron rwaldron approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@FrankYFTang @rwaldron @anba @ptomato