Add getISOCalendarFields() method to calendar-dependent types

[ptomato]

This is in order to be able to get the underlying fields from the data
model, which are stored in the ISO 8601 calendar.

It's not expected to be used in normal Temporal usage, it really exists
only for calendar implementors.

Closes: #354

[codecov]

Codecov Report

Merging #624 into main will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #624      +/-   ##
==========================================
+ Coverage   97.80%   97.82%   +0.02%     
==========================================
  Files          16       16              
  Lines        3691     3727      +36     
  Branches      633      637       +4     
==========================================
+ Hits         3610     3646      +36     
  Misses         79       79              
  Partials        2        2              

Flag	Coverage Δ
#test262	54.51% <100.00%> (+0.20%)	⬆️
#tests	92.79% <88.88%> (-0.04%)	⬇️

Impacted Files	Coverage Δ
polyfill/lib/date.mjs	93.80% <100.00%> (+0.22%)	⬆️
polyfill/lib/datetime.mjs	95.62% <100.00%> (+0.14%)	⬆️
polyfill/lib/monthday.mjs	99.07% <100.00%> (+0.06%)	⬆️
polyfill/lib/yearmonth.mjs	97.31% <100.00%> (+0.10%)	⬆️

---

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 6353cb0...3e6f496. Read the comment docs.

[sffc]

Why the new name? I assume it's because getISOFields() sounds too generic like people would call it even if they didn't mean to do so?

Bikeshed of alternative names:

• getISOSlots()
• getSlots()
• getISOData()
• getISOFieldData()
• getRawFields()
• getRawISOFields()

[ptomato]

The idea for the new name happened at the bottom of #622, and I thought I'd see if anyone didn't like it 😄

Any of those alternatives except getSlots() seem fine with me, on the grounds that they are "scary" enough that they wouldn't be called by accident. All else being equal I think getISOCalendarFields() is probably still the most descriptive though. @justingrant ?

[sffc]

To me, the main purpose of this method is to get at the internal data model, the same one used in the constructor. That's where names like "getRawFields()" and "getSlots()" came from. The fact that the fields are encoded in the ISO calendar is a coincidence. "getISOCalendarFields()" sounds like this method has to do with calling into the calendar API, when in reality, the exact opposite is true!

[ptomato]

Do you have any other comments besides the name? If there's no other obvious candidate for the name, I'm happy to just go back to getISOFields() and talk about renaming it down the line, but I don't want the discussion about the name to block landing this.

[justingrant]

@sffc the main purpose of this method is to get at the internal data model

Will this method also be used to convert from a non-ISO calendar to ISO values? If not, what API will be used to do that?

@ptomato If there's no other obvious candidate for the name

My main concern with naming is avoiding the word "ISO" to avoid confusing users who think that "ISO" means the ISO 8601 string formats. If this API is only for internal/advanced use, then how about getInternalData or getInternalFields? "Internal" is always a good signal that novices should stay away. But any name that doesn't use "ISO" is probably OK.

[sffc]

Do you have any other comments besides the name? If there's no other obvious candidate for the name, I'm happy to just go back to getISOFields() and talk about renaming it down the line, but I don't want the discussion about the name to block landing this.

I haven't reviewed the PR yet besides the name, but I'll do that next.

@sffc the main purpose of this method is to get at the internal data model

Will this method also be used to convert from a non-ISO calendar to ISO values? If not, what API will be used to do that?

The "main" API to do that is intended to be withCalendar():

// Input: hebrewDate is a Temporal.Date that exposes Hebrew fields (year/month/day)
const isoDate = hebrewDate.withCalendar("iso");  // or "iso8601"?
// isoDate is a Temporal.Date that exposes ISO fields instead of Hebrew fields

[sffc]

Another idea/suggestion: how about if we make the schema use the slot names:

// Temporal.Date
{
    isoYear: 2020,
    isoMonth: 5,
    isoDay: 27,
    calendar: ***,
}

// Temporal.DateTime
{
    isoYear: 2020,
    isoMonth: 5,
    isoDay: 27,
    hour: 0,
    minute: 0,
    second: 0,
    millisecond: 0,
    microsecond: 0,
    nanosecond: 0,
    calendar: ***,
}

// Temporal.MonthDay
{
    isoMonth: 5,
    isoDay: 27,
    refYear: 1972,
    calendar: ***,
}

And then the method can be called one of:

• getSlots() -- @ptomato doesn't like this one
• getRawData()
• getRawFields()
• getRepresentation()
• getECMASlots()

[justingrant]

@sffc, I think it'd be a good idea (as you're proposing) to make the internal field names different from the regular field names, so that users won't accidentally think that they're interchangeable.

Also, could we add getInternalData or getInternalFields to the list of choices? If this is an internal-use-only API, then the word "Internal" is helpful to signal to novices to stay away. I think "Raw" achieves a similar purpose but is a little less clear than "Internal".

getRepresentation seems too vague-- it might pique the curiosity of newbies which we probably don't want. getInternalRepresentation would be clearer, but it's very long.

getECMASlots introduces two new concepts "ECMA" and "slots" which might require documenting. Seems like we could instead pick a generic word for "newbies stay away" without introducing a new term to the public API. I'd avoid from getSlots too for the same reason.

[sffc]

getInternalData() or getInternalFields() are fine with me, although "internal" to me suggests that these fields may be unstable and should not be touched in any circumstance, like private fields. However, these fields are well-defined and stable, and they should be used when implementing a custom calendar.

[justingrant]

suggests that these fields may be unstable and should not be touched in any circumstance

Sounds like a feature, not a bug. ;-) Seriously, having a name that discourages most people from touching this method is probably a good thing. Developers who build custom calendars will be advanced experts and will read the docs carefully, so whatever the method is named it won't scare them. A higher priority, IMHO, is scaring the other 99.9% of developers who won't be creating custom calendars.

If using "Internal" is not OK, then I think either "Raw" option would be an OK second choice.

[ryzokuken]

Patch overall LGTM. I was thinking this would be getISOFields along the lines of getFields but I have no strong preferences apart from the strong preferences to get this merged ASAP 😄

[ptomato]

I propose going with getInternalFields for now, but I'm happy to revisit this before Stage 3.

[ryzokuken]

I think me @sffc and @justingrant have different opinions regarding the naming, so I would propose that we move ahead with the current name and bikeshed the name before Stage 3 on a separate issue. @sffc would you like to review this still, or should I merge it in?

[sffc]

Approved with the understanding that the names of things is still an open question. Can you open an issue to track that?

[ptomato]

Was opening #630 as we speak!

[ptomato]

The test failure was a rebase collision with the removal of Temporal.MonthDay.compare.