Add YearInfo to CalendarArithmetic (but don't use it yet in any calendar) #4407
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Manishearth
commented
Dec 5, 2023
| @@ -32,7 +32,7 @@ use core::num::NonZeroU8; | |||
| /// The trait ChineseBased is used by Chinese-based calendars to perform computations shared by such calendar. | |||
| /// | |||
| /// For an example of how to use this trait, see `impl ChineseBasedWithDataLoading for Chinese` in [`Chinese`]. | |||
| pub(crate) trait ChineseBasedWithDataLoading: CalendarArithmetic { | |||
| pub(crate) trait ChineseBasedWithDataLoading: Calendar { | |||
There was a problem hiding this comment.
this had to change, the cyclic dep from ChineseBasedWithDataLoading: CalendarArithmetic and impl<C: ChineseBasedWithDataLoading> CalendarArithmetic for C is brittle and rustc seems to not be willing to resolve certain things there.
We don't use it consistently. If we need to split things later in the future we should do that refactoring then.
|
Argh, I forgot a crucial step here: making CalendarArithmetic work on or at least accepting a YearInfo and now it's somewhat too late: I already got to work on the chinese stuff. This might get bitrotty. |
14 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is mostly a scaffolding PR implementing the first few steps of #3933 (comment).
This:
.offset()to be called with an&impl PrecomputedDataSource<YearInfo>. An intermediate state of the PR had a separate PrecomputedDataSource associated type as per the original design, but I realized it wasn't necessary and it had a tendency to lead to worse trait resolution issues with the ChineseBased blanket impl(). A PR actually using this can add the APIs it needsnew_with_lunar_ordinals(). We weren't maintaining that distinction correctly; and I considered fixing it, but that method only exists in the case of future needs and when those needs come we can easily re-add it and do the proper refactoring.Can be reviewed commit-by-commit if desired, though I'd recommend reviewing the whole PR at once. It's probably sufficient to review calendar_arithmetic.rs and chinese_based.rs.