Remove CalendarArithmetic trait
#7088
Conversation
|
what changes? this should not produce much churn |
There was a problem hiding this comment.
The original intent of the trait was to reduce Calendar impls to "impl CalendarArithmetic and then copy this boilerplate". Ideally, we would have had a forwarding macro.
In practice, that's not happened, and individual impls have done different things when it came to wrapping impls/etc. Since your changes to unify calendar impls with AbstractGregorian/Hijri/LunarChinese, the utility of CalendarArithmetic is greatly reduced.
So overall I think this is a good change. It cleans up single-purpose impls, but the tradeoff is that more of the Calendar methods now need to have thought put into them when implementing. This seems fine to me, it's moving responsibility around.
@sffc had done some weighting of tradeoffs when designing DateFieldsResolver and making it a separate trait, so I want him to sign off on the general approach too.
I did verify that the Indian code, which is the largest change here, is doing the same thing as before.
| #[derive(Debug)] | ||
| #[allow(clippy::exhaustive_structs)] // this type is stable | ||
| pub(crate) struct ArithmeticDate<C: CalendarArithmetic> { | ||
| pub(crate) struct ArithmeticDate<C: DateFieldsResolver> { |
There was a problem hiding this comment.
thought: I think @sffc considered merging DFR into CalendarArithmetic and decided against it.
This PR effectively does that.
DFR is an internal trait but we should consider renaming it now that it's more general
|
@sffc this is a big cleanup PR that is ready to merge. I'd rather not let this go stale over some potential conflict with yet-to-be-created PRs. You cannot have an exclusive lock on the calendar crate. |
|
n.b. I posted my review after Shane's, but I wrote most of it before Shane's, I had not seen his comment. Personally I think we should wait to see if he has an opinion on the merging of CA and DFR. I do not think we should wait on merging this because of unopened PRs, since it becomes impossible to try and reason about merge conflicts without knowing the code being talked about. I think it's reasonable to ask for holding off on merging for conflict reasons, but it's also reasonable to not agree with it. I also don't think this is that much churn. |
|
#7100 is probably one of those PRs. I am okay with having merge order negotiation for opened PRs because it's easy to see what is going on and to adapt. For to-be-opened PRs I think making the request is fine, but it should not block. I do not want to merge this if there is a chance that we'll want to revert it later when there are big PRs floating around since reverting will be painful, this is not a simple refactor. |
|
Process-wise, on a 24-hour review cycle, I wasn't going to hold back on writing the release-critical (because of API impact) #7100 for 1-2 days while we reviewed and iterated on this internal refactoring churn, especially because I wasn't aware that it was coming. I'm going on vacation next week and I want all my 2.1 blockers to be landed. |
|
I think "we should prioritize a likely-conflicting 2.1 blocker over an internal refactor" is a valid reason to provide. I think it's important to provide it, though, because it gives Robert a chance to let us know if this is a prerequisite for any other P1 work, and it gives him a chance to preemptively rebase over the specific PR if he wants. (In general I would prefer if fewer changes were a surprise, too.) I also think that "I have a P1 PR" does not block reviewing. It's perfectly okay to perform review and ask for it to not be landed: the main problem with review latency is iteration velocity, not the actual time til landing. "I am going on vacation next week and have to prioritize landing P1s and cannot review" is still an okay justification as well. But if you do have time to review, it would be preferable to review even if you do not want this to land first because it moves us forward. In short, PR priority is a reason to block merging (not review). Lack of time when on a short deadline is a separate reason to decline review. This might seem like a nitpick but maintaining this distinction in the future and being clear about it helps the rest of the team plan their work. @sffc if you could answer my question about how much you care about DateFieldsResolver and CalendarArithmetic being separate we might be able to make progress here. But perhaps that question can only be answered by looking at this PR in depth. Aside: IMO let's not talk on the tracker in terms of policy discussions that you and I, and Robert and I have had individually: our discussions were different and I was not attempting to be an intermediary so the terminology we have used or the policy we have come up with will mean slightly different things. I would prefer to avoid misunderstandings coming from the fact that "24hr review cycle" was a vehicle for policy discussion that was used differently in our respective discussions. |
I don't think the current line is in a particularly principled place, but I think we should investigate whether we could have many smaller traits as an alternative to one big mega trait. |
|
Thanks! I think currently this replaces one large trait and one small trait with one slightly less small trait. I think that's a big plus. We could split it further if we want but it's a good place to be. The tradeoff is that more of the Calendar methods need manual impl. But we've barely made use of the Calendar methods being boilerplate forwards so I think overall these methods moving to Calendar is simpler. |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a nice refactoring that removes the CalendarArithmetic trait, simplifying the calendar implementations. The logic has been moved to DateFieldsResolver or directly into Calendar trait methods, which reduces indirection and in some cases allows for performance improvements by using direct calculations instead of loops. The changes are well-executed across all affected calendar implementations. I have one suggestion to further improve the code by inlining a calculation to avoid creating a temporary object.
components/calendar/src/date.rs
Outdated
| AbstractGregorian(IsoEra).days_in_year(&ArithmeticDate::new_unchecked( | ||
| self.inner.0.year.saturating_sub(1), | ||
| ), | ||
| 1, | ||
| 1, | ||
| )), |
There was a problem hiding this comment.
Instead of creating a dummy ArithmeticDate to call days_in_year, we can inline the logic directly. This is more efficient and readable, as the logic for calculating days in a Gregorian year is simple and stable.
| AbstractGregorian(IsoEra).days_in_year(&ArithmeticDate::new_unchecked( | |
| self.inner.0.year.saturating_sub(1), | |
| ), | |
| 1, | |
| 1, | |
| )), | |
| 365 + calendrical_calculations::gregorian::is_leap_year(self.inner.0.year.saturating_sub(1)) as u16, |
|
Based on the OP (#7088 (comment)), @Manishearth's summary (#7088 (comment)), and Gemini's summary (#7088 (review)), and without having opened the Files pane, the nature of this PR seems fine to me. Sorry for the blockage yesterday; it was just very bad timing for me. Looking at my outstanding issues, I don't anticipate more large cross-cutting changes after #7105. |
3 participants
Most methods on this trait are only called by the calendar's
Calendarimplementation, or are not called at all. Only two methods are used by the from-fields code, which I moved to theDateFieldsResolvertrait.Not indirecting the logic through 3 intermediate functions allowed for some improvements.