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

ISOMonthDayFromFields ignores year when monthCode is present #2497

Closed
gibson042 opened this issue Feb 3, 2023 · 1 comment · Fixed by #2500
Closed

ISOMonthDayFromFields ignores year when monthCode is present #2497

gibson042 opened this issue Feb 3, 2023 · 1 comment · Fixed by #2500
Labels
behavior Relating to behavior defined in the proposal bug calendar Part of the effort for Temporal Calendar API normative Would be a normative change to the proposal

Comments

@gibson042
Copy link
Collaborator

While working on #2466, I discovered this surprising and inconsistent behavior:

const isoCal = Temporal.Calendar.from("iso8601");
isoCal.monthDayFromFields({ monthCode: "M02", day: 29, year: 2023 }, { overflow: "reject" });
// => Temporal.PlainMonthDay <02-29>
isoCal.monthDayFromFields({ month: 2, day: 29, year: 2023 }, { overflow: "reject" });
// => RangeError (value out of range: 1 <= 29 <= 28)

Since 2023 is not a leap year, both cases should result in a RangeError.

I'm expecting to fix this along with #2466, but it still merits a distinct issue.

@gibson042 gibson042 added bug behavior Relating to behavior defined in the proposal calendar Part of the effort for Temporal Calendar API normative Would be a normative change to the proposal labels Feb 3, 2023
gibson042 added a commit to gibson042/proposal-temporal that referenced this issue Feb 3, 2023
@justingrant
Copy link
Collaborator

justingrant commented Feb 3, 2023

We may choose to change this behavior. But I think the current behavior may actually be consistent with other parts of Temporal, which also follow the following rules:

  • When interpreting property bag inputs, only validate the fields that could contribute to the result; ignore fields in the input that will never affect the result.
  • "Could contribute to the result" should be determined by the shape (presence and types of properties) in the input bag, not the values of those properties. This aligns with intuition from @pipobscure and others to focus validation on the shape of the input (which is determined by the programmer, so bugs will likely be caught during development) rather than the values in the input (which is often determined by the end user). By throwing in all cases where a bag of a particular shape could be invalid, it increases the chance that bugs will be caught during development.

For example, the following code also doesn't throw:

Temporal.PlainTime.from({hour: 12, year: 2023, month: 2, day: 29})

What makes PlainMonthDay a bit weird is that "fields that contribute to the result" can vary more than other types. If there's a monthCode present in the input, then monthCode and day are sufficient to calculate the result. But if there's no monthCode, then month can mean a different real-world month in lunisolar calendars if the month is later in the year than a leap month.

We could have designed the algorithm to only require a year for only lunisolar calendars, or even more narrowly only in lunisolar calendars where month is larger than the earliest possible leap month. But we chose to design it more restrictively so that code written for one calendar was more likely to work with code written for another calendar.

I wouldn't object to also validating the full date whether or not monthCode is present, because prohibiting having an invalid date doesn't seem like it will hurt any valid use cases. But I also don't think we must make this change.

gibson042 added a commit to gibson042/proposal-temporal that referenced this issue Feb 18, 2023
gibson042 added a commit to gibson042/proposal-temporal that referenced this issue Feb 18, 2023
gibson042 added a commit to gibson042/proposal-temporal that referenced this issue Feb 19, 2023
gibson042 added a commit to gibson042/proposal-temporal that referenced this issue Feb 26, 2023
ptomato pushed a commit to gibson042/proposal-temporal that referenced this issue Feb 27, 2023
ptomato pushed a commit to gibson042/proposal-temporal that referenced this issue Apr 18, 2023
gibson042 added a commit to gibson042/proposal-temporal that referenced this issue Apr 19, 2023
Aditi-1400 pushed a commit to Aditi-1400/proposal-temporal that referenced this issue May 17, 2023
ptomato pushed a commit to gibson042/proposal-temporal that referenced this issue May 19, 2023
gibson042 added a commit to gibson042/proposal-temporal that referenced this issue Aug 15, 2023
gibson042 added a commit to gibson042/proposal-temporal that referenced this issue Aug 17, 2023
ptomato pushed a commit to gibson042/proposal-temporal that referenced this issue Aug 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
behavior Relating to behavior defined in the proposal bug calendar Part of the effort for Temporal Calendar API normative Would be a normative change to the proposal
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants