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

Should calendar methods like dateFromFields be able to accept calendar-specific options? #1253

Closed
justingrant opened this issue Jan 9, 2021 · 8 comments · Fixed by #1328
Closed
Assignees
Labels
documentation Additions to documentation non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! spec-text Specification text involved
Milestone

Comments

@justingrant
Copy link
Collaborator

Currently, the options bag for methods like from and with is normalized before the calendar gets ahold of the user's input. This means that calendar developers cannot define custom options. For example, a calendar that spans the Julian=>Gregorian transition can't add an option that offers 'earlier' | 'later' | 'reject' behavior to determine how to handle skipped days.

Should we delegate validation and normalization of options bags to the calendar?

This is analogous to #1229 which proposes delegating validation of input bags to calendars.

Ordinarily I'd say punt this low-priority thing until V2, but one challenge with doing so is that if we did that, then there'd be no way for a calendar to choose a different default value for the overflow option, because the default is pre-filled to 'constrain' before the calendar is called. We still might want to punt this to V2 (or never!) but we'd need to be OK with fixing the default of overflow forever.

@justingrant justingrant changed the title Should calendars be able to accept calendar-specific options? Should calendar methods like dateFromFields be able to accept calendar-specific options? Jan 9, 2021
@Louis-Aime
Copy link

From the user's point of view, the constrain default value for the overflow option gives some comfort that any non-existing 31th day of a month, or 29 Feb., or month 13, shall be converted into a reasonable existing date of the same month, and not silently transformed into a day in another month (or even year) as it is done presently with Date. The user can still use the reject option if he wants a better control.

The Julian => Gregorian transition, as well as Feb. 30 1712 of the Swedish calendar, remain exceptions. IMHO one should not define a general option handling mechanism for this sole usage.

Personally I used the constrain option value in the following way for the western calendar class (class of calendars with Julian to Gregorian transition at a specified date): if fields fall among the skipped dates, compute the day as if the Julian calendar had continued until the switching date's eve in Julian, E.g. in Rome, 14 Oct. 1582 (Julian) yields 1582-10-24. But another author could have constrained all those date expressions to 1582-10-14, the last day of the Julian calendar. In all cases, the user may have a more strict control in setting overflow to reject: trying to specify a skipped date or any non-existing date will throw.

In a few word: leave the calendar's author the ability to define a (unique) constrain mechanism, let the user the ability to reject any illegal date expression. If a special control feature is necessary, the author will define a dedicated custom calendar !

@Louis-Aime
Copy link

Thank you for the comment on #1245 about how the gregory built-in calendar. No one is wrong for not having my opinion, neither for telling me that this comment should come to another place.
Coming specifically to the issue here, I still think that there is no need for opening custom options in Temporal.calendar for such particular cases as Julian => Gregorian transition. In building a specific calendar class (not a subclass), it is possible to handle almost all cases of switching to Gregorian that took place in western Europe. If a special treatment of "skipped days" was necessary, I believe the calendar's author could define one or several variant classes with specific dateFromFields().

@ptomato
Copy link
Collaborator

ptomato commented Jan 15, 2021

Meeting, 14 Jan: We decided to do this.

@ptomato ptomato added documentation Additions to documentation non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! spec-text Specification text involved labels Jan 15, 2021
@ptomato ptomato added this to the Stage 3 milestone Jan 15, 2021
@ptomato ptomato self-assigned this Jan 25, 2021
@ptomato
Copy link
Collaborator

ptomato commented Jan 25, 2021

I don't think this is an objection, but just for clarity: this means that Temporal.X.from(otherTemporalObject, { overflow: 'nonsense' }) and Temporal.X.from('2000-01-01T00:00', { overflow: 'nonsense' }) will no longer throw because they don't call through a calendar method.

@ptomato
Copy link
Collaborator

ptomato commented Jan 25, 2021

Also note that we cannot do the same for dateUntil() because the default value for largestUnit depends on which Temporal type it is used from.

@justingrant
Copy link
Collaborator Author

I don't think this is an objection, but just for clarity: this means that Temporal.X.from(otherTemporalObject, { overflow: 'nonsense' }) and Temporal.X.from('2000-01-01T00:00', { overflow: 'nonsense' }) will no longer throw because they don't call through a calendar method.

Is this limitation really required? Could we apply validation in Temporal core if there's no calendar being called, but delegate to the calendar otherwise?

Also note that we cannot do the same for dateUntil() because the default value for largestUnit depends on which Temporal type it is used from.

Do you mean that there will be validation for dateUntil but user-defined options won't be allowed for that method? Or that there will be no validation for dateUntil because the defaults vary by something the calendar won't know?

Also: how concerned should we be that adding new options in Temporal V2 could break existing calendars that happened to use the same name for their options properties? Should we require all custom options to have a common prefix like is done with X- for custom HTTP headers or custom data- attributes in HTML?

@ptomato
Copy link
Collaborator

ptomato commented Jan 26, 2021

Is this limitation really required? Could we apply validation in Temporal core if there's no calendar being called, but delegate to the calendar otherwise?

OK, I don't see why not.

Do you mean that there will be validation for dateUntil but user-defined options won't be allowed for that method? Or that there will be no validation for dateUntil because the defaults vary by something the calendar won't know?

Neither — I mean that in many cases we will have to create a new object to pass in to dateUntil, and copy all the user-defined properties onto it. For example, if a user passes { myprop: 'myval' } as the options from YearMonth.until(), they expect largestUnit to be 'months', but if you pass the user's object directly into dateUntil, you'll get 'days'. We also cannot mutate the user's object to add a largestUnit property, because for one thing it may not even be possible; the object could be frozen. IMO the only course of action (unless we do disallow user-defined options, but we already said we didn't want to do that) is to loop over the object's own properties and copy them, with possible side effects.

@justingrant
Copy link
Collaborator Author

I mean that in many cases we will have to create a new object to pass in to dateUntil, and copy all the user-defined properties onto it.

OK, this sounds good as long as it's documented. IMHO, anyone who passes an options bag with getters that have side effects deserves to have weird things happen. ;-)

ptomato added a commit that referenced this issue Jan 26, 2021
Instead of creating new objects and copying the options onto them, always
pass the options bags directly through to the Calendar method.

In the case of dateUntil(), when it is necessary to overwrite the
largestUnit option, copy over all own properties of the original options
object instead of setting only the largestUnit property.

Adds a few validate-and-discard steps where options values would otherwise
not be validated.

Closes: #1253
ptomato added a commit that referenced this issue Jan 26, 2021
Instead of creating new objects and copying the options onto them, always
pass the options bags directly through to the Calendar method.

In the case of dateUntil(), when it is necessary to overwrite the
largestUnit option, copy over all own properties of the original options
object instead of setting only the largestUnit property.

Adds a few validate-and-discard steps where options values would otherwise
not be validated.

Closes: #1253
ptomato added a commit that referenced this issue Jan 26, 2021
Instead of creating new objects and copying the options onto them, always
pass the options bags directly through to the Calendar method.

In the case of dateUntil(), when it is necessary to overwrite the
largestUnit option, copy over all own properties of the original options
object instead of setting only the largestUnit property.

Adds a few validate-and-discard steps where options values would otherwise
not be validated.

Closes: #1253
ptomato added a commit that referenced this issue Jan 26, 2021
Instead of creating new objects and copying the options onto them, always
pass the options bags directly through to the Calendar method.

In the case of dateUntil(), when it is necessary to overwrite the
largestUnit option, copy over all own properties of the original options
object instead of setting only the largestUnit property.

Adds a few validate-and-discard steps where options values would otherwise
not be validated.

Closes: #1253
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Additions to documentation non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! spec-text Specification text involved
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants