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.

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

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

Closed
ptomato opened this issue Oct 23, 2020 · 6 comments
Closed

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

ptomato opened this issue Oct 23, 2020 · 6 comments
Milestone

Comments

@ptomato
Copy link
Collaborator

ptomato commented Oct 23, 2020

Follow up from #1014 (comment)

This is something we'll probably have to test in the test262 tests, and make sure that something sensible happens.

@ptomato ptomato added this to the Stage 4 milestone Oct 23, 2020
@ptomato
Copy link
Collaborator Author

ptomato commented Oct 26, 2020

Also mentioned in #1053 (comment) - what if it returns one of the built-in fields that wasn't in the input array?

@justingrant
Copy link
Collaborator

Related is #1054:

  1. Why does fields require returning all fields? Are calendars allowed to remove built-in fields? If not, why not just additional ones, so implementation would be simpler and it'd avoid error cases because we could simply ignore or throw when we see a built-in fields.
  2. Why is fields a method instead of a property returning an object like below? See Calendar.prototype.fields feedback #1054 for reasons why this could be helpful, e.g. reduce errors, easier debugging, etc.
{
  plainDate: ['era'],
  plainYearMonth: ['era'],
  // plainMonthDay: undefined, 
  // plainTime: undefined, 
  zonedDateTime:  ['era'],
  plainDateTime: ['era']
}
  1. Why is it called fields instead of another name that isn't confusable with getFields and that clarifies that only names, not values, are returned? For example, extraFieldNames.

@ptomato
Copy link
Collaborator Author

ptomato commented Oct 27, 2020

I think we can stick to #1054 for those, the questions above still apply even if those changes are made.

@ptomato ptomato modified the milestones: Stage 4, Next Feb 18, 2021
@cjtenny cjtenny mentioned this issue Feb 27, 2021
44 tasks
@ptomato ptomato modified the milestones: Next, Stage "3.5" Dec 8, 2022
@ptomato
Copy link
Collaborator Author

ptomato commented Dec 8, 2022

Pretty sure the answer is that we do something sensible in that case. Verify and close

@gibson042
Copy link
Collaborator

It looks like the "fields" method is only invoked from CalendarFields, whose output is always fed to PrepareTemporalFields which propagates it back to end up in CalendarYearMonthFromFields, CalendarMonthDayFromFields, CalendarDateFromFields, or InterpretTemporalDateTimeFields. The first three send fields right back to the corresponding calendar method, while the last sends fields to ToTemporalTimeRecord (which invokes PrepareTemporalFields itself and then looks only for specific fields in the result) and then to the already-covered CalendarDateFromFields. I think we're good.

@ptomato
Copy link
Collaborator Author

ptomato commented Dec 17, 2022

Thanks for looking into it. Closing 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants