Calendar.fields method #1014

ptomato · 2020-10-19T15:59:04Z

Closes: #666

codecov · 2020-10-19T16:10:38Z

Codecov Report

Merging #1014 into main will decrease coverage by 0.08%.
The diff coverage is 90.78%.

@@            Coverage Diff             @@
##             main    #1014      +/-   ##
==========================================
- Coverage   94.38%   94.30%   -0.09%     
==========================================
  Files          18       18              
  Lines        6556     6657     +101     
  Branches      983      998      +15     
==========================================
+ Hits         6188     6278      +90     
- Misses        361      372      +11     
  Partials        7        7

Flag	Coverage Δ
#test262	`48.05% <73.50%> (+0.45%)`	⬆️
#tests	`90.43% <84.21%> (-0.12%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
polyfill/lib/datetime.mjs	`94.98% <66.66%> (-0.26%)`	⬇️
polyfill/lib/date.mjs	`91.96% <83.33%> (-0.19%)`	⬇️
polyfill/lib/ecmascript.mjs	`95.98% <91.01%> (-0.20%)`	⬇️
polyfill/lib/calendar.mjs	`75.50% <100.00%> (+0.31%)`	⬆️
polyfill/lib/monthday.mjs	`92.76% <100.00%> (+0.24%)`	⬆️
polyfill/lib/yearmonth.mjs	`95.10% <100.00%> (+0.14%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 43b8373...292b17f. Read the comment docs.

ptomato · 2020-10-19T16:33:32Z

Sorry for the noise. This seems to mysteriously fail the test suite with no message (even though all the tests pass). I will investigate this before re-requesting review.

ptomato · 2020-10-19T17:19:56Z

demitasse seems to swallow exceptions in after() blocks, they still fail the test suite, but print no message. I've fixed the exception now, it's ready for review again.

@pipobscure I don't have time right now to investigate whether this still happens in demitasse 2.x, so I'm inclined to leave it, but if you prefer I can open an issue in demitasse so it doesn't get lost.

docs/calendar.md

pipobscure · 2020-10-19T19:12:42Z

@pipobscure I don't have time right now to investigate whether this still happens in demitasse 2.x, so I'm inclined to leave it, but if you prefer I can open an issue in demitasse so it doesn't get lost.

Please do. I have to go over that in a few weeks; so I’ll certainly have forgotten this by then.

polyfill/lib/date.mjs

Ms2ger · 2020-10-21T13:26:15Z

polyfill/lib/date.mjs

+    const fields = ES.ToTemporalDateFields(this, calendar);
+    fields.calendar = calendar;


What if fields() returns "calendar" as one of the fields?

I'm not sure what should be done in that case; I guess one option is to throw if calendar is returned in the validation step I mentioned in #1014 (comment). The other option is to 🤷 and say "well, don't do that then." I'm mildly in favour of the latter since I think it's in line with what we do elsewhere for custom calendars and time zones.

polyfill/lib/ecmascript.mjs

Ms2ger · 2020-10-21T13:44:16Z

spec/date.html

+          1. If _property_ is not *"era"*, then
+            1. Let _value_ be ? ToInteger(_value_).


This makes no sense to me: this would convert any custom property to an integer?

That is what I intended. The alternative is to treat any custom property just like era and leave it unchanged. My reasoning was that we wouldn't want to encourage more non-numerical properties like era, but I don't have a strong opinion on which is correct.

We shouldn't assert a particular type on custom properties, except perhaps requiring that it is a primitive.

I don't have a clear idea of the use case of this feature, but this seems to make it impossible to use non-integer properties, rather than just discourage. I'll merge now in the interest of moving forward, but filed #1045.

These are not done yet, but in order for the spec changes for the fields() method to make sense, they at least need to be marked as such. See: #666

This is reflected in the polyfill but not yet in the spec. It's also not complete, since it will change anyway due to #522, but it's enough to make the spec changes for calendar.fields make sense. See: #666

The Calendar.fields method is called whenever Temporal needs to determine if a calendar object requires extra fields to uniquely identify its date. Closes: #666

ptomato requested review from Ms2ger, justingrant, pipobscure, jasonwilliams, sffc, gibson042, mattjohnsonpint and ryzokuken October 19, 2020 15:59

ptomato force-pushed the 666-calendar-fields branch from acb96bf to 744711a Compare October 19, 2020 16:31

ptomato marked this pull request as draft October 19, 2020 16:32

ptomato removed request for Ms2ger, justingrant, pipobscure, jasonwilliams, sffc, gibson042, mattjohnsonpint and ryzokuken October 19, 2020 16:32

ptomato force-pushed the 666-calendar-fields branch from 744711a to 113fa29 Compare October 19, 2020 17:18

ptomato marked this pull request as ready for review October 19, 2020 17:20

ptomato requested review from Ms2ger, ryzokuken, sffc, gibson042, jasonwilliams, justingrant and mattjohnsonpint October 19, 2020 17:20

ptomato requested a review from pipobscure October 19, 2020 17:20

sffc reviewed Oct 19, 2020

View reviewed changes

docs/calendar.md Show resolved Hide resolved

Ms2ger requested changes Oct 21, 2020

View reviewed changes

Ms2ger added the awaiting-author label Oct 21, 2020

ptomato added 2 commits October 22, 2020 11:51

Add various calendar TODOs to spec

6c28050

These are not done yet, but in order for the spec changes for the fields() method to make sense, they at least need to be marked as such. See: #666

Split date and time operations in DateTime.with()

b3bd658

This is reflected in the polyfill but not yet in the spec. It's also not complete, since it will change anyway due to #522, but it's enough to make the spec changes for calendar.fields make sense. See: #666

ptomato force-pushed the 666-calendar-fields branch from 113fa29 to 9dcf449 Compare October 22, 2020 21:59

ptomato removed the awaiting-author label Oct 22, 2020

ptomato requested a review from Ms2ger October 22, 2020 22:00

Add Calendar.fields method

292b17f

The Calendar.fields method is called whenever Temporal needs to determine if a calendar object requires extra fields to uniquely identify its date. Closes: #666

ptomato force-pushed the 666-calendar-fields branch from 9dcf449 to 292b17f Compare October 22, 2020 23:44

Ms2ger approved these changes Oct 23, 2020

View reviewed changes

Ms2ger mentioned this pull request Oct 23, 2020

Don't convert custom fields from calendar.fields() to integers #1045

Closed

Ms2ger merged commit efdb545 into main Oct 23, 2020

Ms2ger deleted the 666-calendar-fields branch October 23, 2020 08:51

ptomato mentioned this pull request Oct 23, 2020

What if calendar.fields() returns "calendar" as one of the fields? #1047

Closed

justingrant mentioned this pull request Oct 24, 2020

Calendar.prototype.fields feedback #1054

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Calendar.fields method #1014

Calendar.fields method #1014

ptomato commented Oct 19, 2020

codecov bot commented Oct 19, 2020 •

edited

Loading

ptomato commented Oct 19, 2020

ptomato commented Oct 19, 2020

pipobscure commented Oct 19, 2020

Ms2ger Oct 21, 2020

ptomato Oct 22, 2020

ptomato Oct 23, 2020

Ms2ger Oct 21, 2020

ptomato Oct 22, 2020

sffc Oct 23, 2020

Ms2ger Oct 23, 2020

		const fields = ES.ToTemporalDateFields(this, calendar);
		fields.calendar = calendar;

		1. If _property_ is not "era", then
		1. Let _value_ be ? ToInteger(_value_).

Calendar.fields method #1014

Calendar.fields method #1014

Conversation

ptomato commented Oct 19, 2020

codecov bot commented Oct 19, 2020 • edited Loading

Codecov Report

ptomato commented Oct 19, 2020

ptomato commented Oct 19, 2020

pipobscure commented Oct 19, 2020

Ms2ger Oct 21, 2020

Choose a reason for hiding this comment

ptomato Oct 22, 2020

Choose a reason for hiding this comment

ptomato Oct 23, 2020

Choose a reason for hiding this comment

Ms2ger Oct 21, 2020

Choose a reason for hiding this comment

ptomato Oct 22, 2020

Choose a reason for hiding this comment

sffc Oct 23, 2020

Choose a reason for hiding this comment

Ms2ger Oct 23, 2020

Choose a reason for hiding this comment

codecov bot commented Oct 19, 2020 •

edited

Loading