Conversation
0f3fa2f to
b32e955
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3292 +/- ##
==========================================
+ Coverage 97.64% 97.83% +0.19%
==========================================
Files 22 22
Lines 10746 10725 -21
Branches 1855 1856 +1
==========================================
Hits 10493 10493
+ Misses 235 215 -20
+ Partials 18 17 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
b32e955 to
5c419c3
Compare
ptomato
left a comment
There was a problem hiding this comment.
Thanks for doing the extra research to try to find a case that would reach this code. I was a bit uneasy removing this because it may have been put there for a reason that's lost to the mists of time (this generic calendar implementation was done in a hurry)
On the other hand, since it corresponds to the implementation-defined part of the spec text, there isn't that much potential downside to removing it, as you point out.
I think the assertNotReached can be simplified into an assert so we don't have to mess around with c8 magic comments, but I'll fix that up before merging.
Thanks!
5c419c3 to
501d4ae
Compare
|
We should fix up the comment as well, since it's no longer a binary search. |
The overshoot block in
calendarToIsoDate's binary search appears unreachable. The closest candidate is Coptic/Ethiopic month 13 (only 5–6 days), where the 8-day step could theoretically skip the month entirely — but even there, I haven't been able to construct an example that triggers it. The refinement step keeps estimates close enough thatcalculateSameMonthResultalways handles them first.Replace it with an
assertNotReachedso we get a clear error if this analysis is ever wrong.Closes #3217