-
Notifications
You must be signed in to change notification settings - Fork 43
Add with to PlainYearMonth #231
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
Changes from 2 commits
99cddae
a8e673f
cd48780
d67f311
867ab63
cf0870f
07be388
ab4d54e
5fcd45a
557e09e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -182,13 +182,35 @@ impl PlainYearMonth { | |
| self.calendar.identifier() | ||
| } | ||
|
|
||
| /// Returns the calendar day value. | ||
| pub fn day(&self) -> TemporalResult<u8> { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was double checking the PR and noticed this method. This should be removed as
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm having an issue with the FFI bindings from boa-dev/temporal to boa. When I run core/engine/src/builtins/temporal/plain_year_month/mod.rs:372:53
let result = year_month.inner.with(partial, overflow)?;
expected `Option<ArithmeticOverflow>`, found `ArithmeticOverflow`I ran
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, that looks like it's the call from You'd need to update the code in Boa. The EDIT: just to note, that failure shouldn't affect this PR. Currently, it looks like you only need to run Rustfmt 😄 |
||
| self.calendar.day(&self.iso) | ||
| } | ||
|
|
||
| /// Creates a `PlainYearMonth` using the fields provided from a [`PartialDate`] | ||
| pub fn with( | ||
| &self, | ||
| _partial: PartialDate, | ||
| _overflow: ArithmeticOverflow, | ||
| partial: PartialDate, | ||
| overflow: Option<ArithmeticOverflow>, | ||
| ) -> TemporalResult<Self> { | ||
| Err(TemporalError::general("Not yet implemented.")) | ||
| // 1. Let yearMonth be the this value. | ||
| // 2. Perform ? RequireInternalSlot(yearMonth, [[InitializedTemporalYearMonth]]). | ||
| // 3. If ? IsPartialTemporalObject(temporalYearMonthLike) is false, throw a TypeError exception. | ||
| if partial.is_empty() { | ||
| return Err(TemporalError::r#type().with_message("A PartialDate must have a field.")); | ||
| }; | ||
| // 4. Let calendar be yearMonth.[[Calendar]]. | ||
| // 5. Let fields be ISODateToFields(calendar, yearMonth.[[ISODate]], year-month). | ||
| // 6. Let partialYearMonth be ? PrepareCalendarFields(calendar, temporalYearMonthLike, « year, month, month-code », « », partial). | ||
| // 7. Set fields to CalendarMergeFields(calendar, fields, partialYearMonth). | ||
| // 8. Let resolvedOptions be ? GetOptionsObject(options). | ||
| // 9. Let overflow be ? GetTemporalOverflowOption(resolvedOptions). | ||
| // 10. Let isoDate be ? CalendarYearMonthFromFields(calendar, fields, overflow). | ||
| // 11. Return ! CreateTemporalYearMonth(isoDate, calendar). | ||
| self.calendar.year_month_from_partial( | ||
| &partial.with_fallback_year_month(self)?, | ||
| overflow.unwrap_or(ArithmeticOverflow::Constrain), | ||
| ) | ||
| } | ||
|
|
||
| /// Compares one `PlainYearMonth` to another `PlainYearMonth` using their | ||
|
|
@@ -300,6 +322,84 @@ mod tests { | |
|
|
||
| use super::PlainYearMonth; | ||
|
|
||
| use tinystr::tinystr; | ||
|
|
||
| use super::*; | ||
|
|
||
| #[test] | ||
| fn test_plain_year_month_with() { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. praise: great tests! |
||
| let base = PlainYearMonth::new_with_overflow( | ||
| 2025, | ||
| 3, | ||
| None, | ||
| Calendar::default(), | ||
| ArithmeticOverflow::Reject, | ||
| ) | ||
| .unwrap(); | ||
|
|
||
| // Year | ||
| let partial = PartialDate { | ||
| year: Some(2001), | ||
| ..Default::default() | ||
| }; | ||
|
|
||
| let with_year = base.with(partial, None).unwrap(); | ||
| assert_eq!(with_year.iso_year(), 2001); // year is changed | ||
| assert_eq!(with_year.iso_month(), 3); // month is not changed | ||
| assert_eq!( | ||
| with_year.month_code().unwrap(), | ||
| MonthCode::from_str("M03").unwrap() | ||
| ); // assert month code has been initialized correctly | ||
|
|
||
| // Month | ||
| let partial = PartialDate { | ||
| month: Some(2), | ||
| ..Default::default() | ||
| }; | ||
| let with_month = base.with(partial, None).unwrap(); | ||
| assert_eq!(with_month.iso_year(), 2025); // year is not changed | ||
| assert_eq!(with_month.iso_month(), 2); // month is changed | ||
| assert_eq!( | ||
| with_month.month_code().unwrap(), | ||
| MonthCode::from_str("M02").unwrap() | ||
| ); // assert month code has changed as well as month | ||
|
|
||
| // Month Code | ||
| let partial = PartialDate { | ||
| month_code: Some(MonthCode(tinystr!(4, "M05"))), // change month to May (5) | ||
| ..Default::default() | ||
| }; | ||
| let with_month_code = base.with(partial, None).unwrap(); | ||
| assert_eq!(with_month_code.iso_year(), 2025); // year is not changed | ||
| assert_eq!( | ||
| with_month_code.month_code().unwrap(), | ||
| MonthCode::from_str("M05").unwrap() | ||
| ); // assert month code has changed | ||
| assert_eq!(with_month_code.iso_month(), 5); // month is changed as well | ||
|
|
||
| // Day | ||
| let partial = PartialDate { | ||
| day: Some(15), | ||
| ..Default::default() | ||
| }; | ||
| let with_day = base.with(partial, None).unwrap(); | ||
| assert_eq!(with_day.iso_year(), 2025); // year is not changed | ||
| assert_eq!(with_day.iso_month(), 3); // month is not changed | ||
| assert_eq!(with_day.iso.day, 15); // day is changed | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. issue: so this one is a little tricky, but the ISO day property shouldn't be touched. For reference, see the basic PlainYearMonth.prototype.with test |
||
|
|
||
| // All | ||
| let partial = PartialDate { | ||
| year: Some(2001), | ||
| month: Some(2), | ||
| day: Some(15), | ||
| ..Default::default() | ||
| }; | ||
| let with_all = base.with(partial, None).unwrap(); | ||
| assert_eq!(with_all.iso_year(), 2001); // year is changed | ||
| assert_eq!(with_all.iso_month(), 2); // month is changed | ||
| assert_eq!(with_all.iso.day, 15); // day is changed | ||
| } | ||
|
|
||
| #[test] | ||
| fn basic_from_str() { | ||
| let valid_strings = [ | ||
|
|
||

Uh oh!
There was an error while loading. Please reload this page.
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.
issue (part 2 on test review comment): so this macro impl specifically on
PartialDateis a bit problematic for specificallyPlainYearMonth.The method steps at play here are 6-7:
PrepareCalendarFieldsis a bit of a complex abstract operations that's main purpose is to pull fields from a JavaScript object. In order to handle this, the partial objects were created.You correctly noted that the partial cannot be empty when submit into
with, but the actual assertion technically occurs here in step 6, not 3 (that abstract operation does ALOT).This method implemented by this macro is
temporal_rs's answer for step 7. The only issue is that day should never be set downstream by the caller due to step 6.The simple solution would be to manually implement a
with_fallback_year_monththat does not touch thepartial.day(like the other year_month method above this).The other solution (that after typing this all out I'm more convinced is the correct approach) is to move that invariant of not touching the day field into the type system and create a
PartialYearMonthstruct that can then be used on allPlainYearMonthpaths that are usingPartialDate.The second solution is probably a bit of scope creep, so if you'd like to go with the first solution, that's fine by me. We can review and merge this, and I'll open up another issue for implementing
PartialYearMonth.If you want to do the second solution, I think
PartialYearMonthshould look something like:Uh oh!
There was an error while loading. Please reload this page.
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.
Another solution could be to make the function return an
Errif the PartialDatedayfield is populated, and then just use the regular macro?But I like the type system approach of
PartialYearMonth. (If you do, it looks like it should go inyear_month.rs)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.
Yeah, the incoming partial would still have to throw on
partail.day.is_some(), but the regular macro still sets the day according to the fallback, which would still be invalid as well I believe. The day field is "resolved" by the calendar, at least according to the specification, in step 2.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.
I’ve implemented a solution that avoids modifying the
dayfield while still using the macro. Instead of implementing a separatewith_fallback_year_monthor type system, I’ve tweaked the macro to require explicit day specification, so PlainYearMonth can use it without setting day.It’s set to None via ..Default::default(), but I’m concerned this doesn’t strictly align with spec step 6 of 9.3.13. Is this okay, or should I continue with the type system instead?
Check out the latest commit. I’ve also changed the test cases which now ignores any attempt to change the
dayof aPlainYearMonth.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.
I saw the change you had made to the macro 😃 It's a pretty creative one!
Strictly aligning to the spec on step 6 is incredibly hard due to its reliance on JavaScript objects, and essentially requires more of an interpretation rather than step by step adherence, which is how the partial objects came to be.
The best way to test is to see if it passes the test262 test suite. Have you run the test262 test suite with Boa by chance to test conformance?
I still have to do a more proper review, but I think I'm fine with merging this current approach for now. But as mentioned above, I think long term moving to a
PartialYearMonthstruct is the best option overall. So a follow up PR to complete that is definitely best.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.
The macro trick was all @sffc—credit goes to him😅. Right now, testing Temporal with Boa doesn’t seem to work for me🤔. I suspect it may relate to the MonthCode change in temporal, I’m not entirely sure. If the fix is trivial, I’ll try and run Test262.
Here are some of the errors i am experiencing
There is a total of 34 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.
Has this been rebased or merged with main recently? I thought more or less Boa was up to date ... but maybe its a bit more out of sync then I had thought. I can have an update complete by tonight.
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.
I've rebased onto the main branch and successfully tested with the test262 suite.
Note: I modified the
get_optioncall in boa, removing the.unwrap_or_default();