Skip to content
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

don't override rule params with start's params #232

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

stratoukos
Copy link

Fixes #231.

I'm not sure if this is the correct approach, but I deleted _event_params and all of it supporting code. As far as I can tell, changing the rule's params using parameters derived from start is wrong and will produce wrong occurrences.

I tried tracing the origins of this function, and it was added in b8920af in a recent pull request (#182) to fix issue #141. Some comments about #141:

  • I can't reproduce the issue (having occurrences after end_recurring_period), neither before nor after my patch.
  • The rule of the event described by the issue is invalid per RFC-5545 (WEEKLY events can't have bymonthday according to this table). Dateutil, however, will accept this and generate occurrences, as documented here (3rd bullet).
  • The problem described in the issue's second comment doesn't seem to be a bug. The RFC does not mention this explicitly, but it's my understanding that an event does not necessarily need to be part of its occurrences. Other calendar applications (tested Google Calendar and OSX's Calendar app) handle this be changing the event's start time to that of its first occurrence[1].

Perhaps @nwaxiomatic can shed some light on _event_params's purpose.

[1]: Add an event on a Monday repeating weekly every Tuesday. You will not see an occurrence on that Monday and if you export to icalendar, the event's start date will be that of the first occurrence, ie the following Tuesday.

@coveralls
Copy link

coveralls commented Jul 29, 2016

Coverage Status

Coverage increased (+1.3%) to 67.979% when pulling 4c7e686 on indevgr:231_dont_override_rrule into 0cb031b on llazzaro:develop.

@stratoukos stratoukos force-pushed the 231_dont_override_rrule branch from 4c7e686 to fb298b9 Compare July 29, 2016 17:35
@stratoukos
Copy link
Author

I forgot to remove a now unused import. Pushed again.

@coveralls
Copy link

coveralls commented Jul 29, 2016

Coverage Status

Coverage increased (+1.3%) to 67.979% when pulling fb298b9 on indevgr:231_dont_override_rrule into 0cb031b on llazzaro:develop.

@symbiosdotwiki
Copy link
Contributor

There were a bunch of cases given the standard rrule usage that produced flat our wrong occurrences.

If you gave it rules that should have no occurrences, it didn't actually check those rules properly and made occurrences. I added an extra layer of rule fact checking to weed out occurrences that didn't follow the rules.

Perhaps there were logical bugs, but I can assure you there are more bugs without it.

See the original test case I mentioned in the first issue and check that it does not give the intuitive set of occurrences.

@llazzaro
Copy link
Owner

@nwaxiomatic let me know if you can get an unit test for your case. I will try to analize more this case.

@stratoukos
Copy link
Author

I tried the original case (start: 2015-08-15, end 2015-09-15, rule: Weekly bymonthday:29,30) and I got back 2015-08-29 and 2015-08-30 as occurrences.

While I can't say if these are correct (since the rule parameters are invalid) they seem reasonable to me and it's what dateutil's rrule generates as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

recurring events with multiple "byweekday"s return wrong occurences
4 participants