-
Notifications
You must be signed in to change notification settings - Fork 188
fix Calendar.RecurrenceRule
#1284
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?
fix Calendar.RecurrenceRule
#1284
Conversation
@@ -232,6 +235,8 @@ extension Calendar { | |||
} | |||
var componentsForEnumerating = recurrence.calendar._dateComponents(components, from: start) | |||
|
|||
startDateNanoseconds = start.timeIntervalSince1970.remainder(dividingBy: 1) |
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.
Would it work if we modify components
above to include nanoseconds so the information is readily available in componentsForEnumerating
, so that we don't have to do this manual adjustment here?
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.
@itingliu that was my first attempt, but it doesn't work.
in my understanding, the issue is that Calendar.DatesByRecurring
's underlying baseRecurrence
(Calendar.DatesByMatching
) will always return only whole-second-rounded dates (though this isn't documented, afaict), so we'd need to adjust that type's behaviour instead (which i decided against bc i wasn't sure whether this would affect any other dependent code, and i figured instead going with the simplest possible fix would be the best approach)
@swift-ci please test |
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.
Fix makes sense to me. @hristost can you take a look as well?
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.
Looks good for me, thank you for finding and fixing this bug!
This PR attempts to fix #1283, by adjisting
Calendar.DatesByRecurring.Iterator
to maintain nanosecond precision on the dates it produces, if the original start date had a non-zero nanosecond component.This brings the behaviour of the
Calendar.RecurrenceRule
API in line with how it was defined and described in the proposal.