-
Notifications
You must be signed in to change notification settings - Fork 244
Clean up FormattableAnyCalendar
#7318
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
|
Can you give an overview? What problem does this solve? /gemini review |
|
Hi @sffc, thanks for the question! This pull request, titled "Clean up Problem Solved:
In essence, this change makes the calendar handling more robust, maintainable, and consistent with the broader Regarding your |
|
/gemini review |
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.
Code Review
This pull request is a nice cleanup of FormattableAnyCalendar. It simplifies the implementation by removing the UntaggedFormattableAnyCalendar wrapper and the cached kind field. The logic for selecting a calendar based on preferences is now encapsulated within the FormattableAnyCalendar constructors. The changes are consistent and improve the overall structure. I have one suggestion to reduce code duplication in components/datetime/src/scaffold/calendar.rs.
sffc
left a comment
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.
If we don't need UntaggedFormattableAnyCalendar any more, I'm in favor of that change, and would prefer to see it in a standalone PR. I'm less convinced by the motivation for the other changes.
| pub(crate) selection: DateTimeZonePatternSelectionData, | ||
| pub(crate) names: RawDateTimeNames<FSet>, | ||
| pub(crate) calendar: UntaggedFormattableAnyCalendar, | ||
| pub(crate) calendar: FormattableAnyCalendar, |
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.
Observation: I think I had introduced UntaggedFormattableAnyCalendar because it reduced the stack size of DateTimeFormatter. But, the tests that enforce stack size are still passing, so maybe there's a new niche where the extra field can fit now.
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.
FormattableAnyCalendar is now just untagged
| // unsupported | ||
| prefs.calendar_algorithm = None; | ||
| return Self::new(prefs); |
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.
Observation/Issue: This logic that used to live in 1 place now lives in 3 places
Trying to reduce usage of
AnyCalendarKindandFormattableAnyCalendarKind.