-
Notifications
You must be signed in to change notification settings - Fork 225
Move DateFields::month_code over to being a byte slice #7116
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
base: main
Are you sure you want to change the base?
Conversation
dd049e8
to
5eae8bf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems good. You only need to do funny number stuff in a few test files which is fine; they don't reflect how real people will be using this.
That was my conclusion too. It's a bit of a bummer to lose the readability of the old new_normal, new_leap APIs though |
I'm overall quite convinced by the "from_fields should accept anything" argument so even if we had ValidMonthCode already designed and graduated I would not want to use it here anyway. |
Also note: this PR doesn't yet fix the Debug impl but it's an easy addition I'll do soon. |
5eae8bf
to
7f4ce11
Compare
(undid the force push, the new Debug commit is the only new thing) |
7f4ce11
to
c2b2dba
Compare
Depends on #7115, depends on #7105
Attempt at fixing #7104.
The user experience of the byte slice is a little bit worse if you want to use MonthCode::new_normal/new_leap since you have to persist those for a while.
Most people will not be creating them for long, though. In theory we could produce byte versions of those methods that produce a limited range of codes. It's more a problem for our tests that need to do things like "create many codes in a loop".