-
Notifications
You must be signed in to change notification settings - Fork 129
Description
Support for all-day events is ugly and confusing.
make_all_day treats an event's begin/end times as an inclusive date range, and converts it to half-open. For begin, it's set to the start of its day, but for end it's bumped to the start of the next day, even if it's already at the start of a day. Additionally, if the begin time is subsequently changed, the event unexpectedly reverts to a not-full-day event. This is not documented and has a side effect that calling make_all_day again causes the end to be bumped up by one, as it is re-converted from inclusive to half-open range. However, setting the end time does not effect the all-day status, nor is the value rounded or bumped; you can make an all-day event that's 1 day, 3 hours and 5 minutes.
I'd like to suggest a combination of behavioral and documentation improvements. I'm willing to commit time to make these happen, but want to gather input and confirm I'm sane in my approach:
Definite code changes:
- If an event has a duration instead of an end time,
make_all_dayshould round the duration up to the next 24-hour interval. If it's already an even day, the duration should be unchanged. (Currently, a duration is converted to a fixed end-time, subject to bumping on successivemake_all_day.)
Definite documentation changes:
- Describe how how start/end times/dates are transformed by
make_all_day.
Possible code changes:
- Instead of having
begin's setter implicitly change events to non-all-day event, instead have it round the provided begin time to the start of day when it's an all-day event. - Modify
end's setter to also round, but round up rather than down. Setting to start of day should not increment to next day (to retain compatibility with current behavior), but that should be clearly documented. - Add a boolean parameter ala
make_all_day(ad = True), with default True which would make the event all-day. If the parameter is false, revert to a non-all-day event.
That seems sane to me, as it's changing already-blurry areas for behavior. But it's still changing behavior and risks breaking code using the library. Alternately, there's the documentation route:
- Document that the event loses it's all-day property if the begin time is set.
- Indicate that
make_all_day()can be called to restore all-day status, but doing so will bump the end date again. Assuming the change for all-day durations is in place, suggest using duration instead of end date to avoid bumping.
Thoughts?