Skip to content

Conversation

sffc
Copy link
Member

@sffc sffc commented Oct 15, 2025

#7010

This adds ValidMonthCode as an internal type.

Suggested in #7100

Copy link
Member

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really want us to switch to a new month code everywhere. When I mentioned ValidMonthCode I was narrowly mentioning it as a potential unstable API for ecma_reference_year.

In the medium term we can consider migrating.

@sffc sffc force-pushed the valid-month-code branch from 8c322ce to fc91514 Compare October 16, 2025 01:19
@sffc sffc force-pushed the valid-month-code branch from fc91514 to d7dce4e Compare October 16, 2025 01:19
@sffc sffc changed the title Add ValidMonthCode Streamline more error cases around month code parsing Oct 16, 2025
@sffc sffc requested a review from Manishearth October 16, 2025 01:54
@sffc
Copy link
Member Author

sffc commented Oct 16, 2025

I made the type fully internal.

For now I'm passing tuples into the Rules traits. We should revisit whether to export ValidMonthCode when we graduate the Rules traits in 2.2.

@sffc sffc marked this pull request as ready for review October 16, 2025 01:55
@sffc sffc requested a review from robertbastian October 16, 2025 01:58
@Manishearth
Copy link
Member

I like this

@sffc
Copy link
Member Author

sffc commented Oct 16, 2025

I'll wait on @robertbastian since he usually has feedback on errors.

autosubmit = true

@sffc
Copy link
Member Author

sffc commented Oct 16, 2025

Note: this PR changes EcmaReferenceYearError (which is internal) to have 2 variants instead of 3 variants. I did try putting it down to 1 variant via Option<Result>, but it made the implementers and call sites substantially more complex in my opinion, so I reverted to keeping 2 variants in the error enum. We could revisit this for the public Rules traits upon graduating them.

@sffc
Copy link
Member Author

sffc commented Oct 16, 2025

This PR is not merged yet but might conflict with #7115 and #7104. Whoever starts work on those PRs should merge this first.

Manishearth added a commit that referenced this pull request Oct 16, 2025
Progress on #7104. I didn't
touch MonthCode yet since it becomes easier after
#7105 lands.
@Manishearth
Copy link
Member

Manishearth commented Oct 16, 2025

@sffc #7115 does not conflict, I already checked locally.

#7116 (the other half of #7104) is built on top of this PR.

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

Successfully merging this pull request may close these issues.

2 participants