-
Notifications
You must be signed in to change notification settings - Fork 16
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
feat: Timeframe Filter #144
Conversation
During the implementation of this, I noticed we could use a "setupFilter" function. This function would be called on the initialization of the filter, allowing us to catch invalid configurations from the user, such as invalid HourStart or HourEnd values. |
|
||
func (a TimeFrameEvents) Filter(event models.Event) bool { | ||
// If the user enters invalid hours, such as numbers higher than 24 or lower than 0, all events will be synchronized up to that point in time. For example, if the start hour is -1 and the end hour is 17, all events until 17 o'clock will be synchronized. | ||
if event.StartTime.Hour() >= a.HourStart && event.StartTime.Hour() <= a.HourEnd { |
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.
I was wondering about the exact semantics we want here. Assuming a time frame from 8-17 we currently get the following behavior:
event start/end | filter |
---|---|
6-7 | false |
7-9 | false (why?) |
10-12 | true |
16-18 | true |
18-19 | false |
6-18 | false (why?) |
I think we want to include all events that overlap with the timeFrame in any way. That is, an event should be included if the event start or end is within the timeFrame but also the other way around if the timeFrame start or end are within the event.
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.
I think we want to include all events that overlap with the timeFrame in any way.
yup, that should be the expected behaviour. Implemented the logic like this and added some more test cases.
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.
Sorry for the delay.
related to #114