Page MenuHomekolab.org

Add RDATE support (#5401)
ClosedPublic

Authored by machniak on Apr 26 2016, 9:57 AM.

Diff Detail

Repository
rP pykolab
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

machniak updated this revision to Diff 289.Apr 26 2016, 9:57 AM
machniak retitled this revision from to Add RDATE support (#5401).
machniak updated this object.
machniak edited the test plan for this revision. (Show Details)
machniak added a reviewer: PyKolab Developers.

Please see the inline commentary.

pykolab/xml/event.py
989

Does the existence of an _rdate.params attribute imply that the 'TZID' key is set?

1367

Should not both clauses in this || statement evaluate to True? It seems that for a recurring event, .isValid() should be true as well as (implicitly) a positive number of occurrences should exist.

Note also that a "recurring" event with only one occurrence would validate len() > 0, which may not yield the expected results.

Also note that self.event (the lower-level XML level) and self.get_recurrence_dates() work on two different in-memory representations of the event, allowing for race-conditions. Perhaps it is best to ensure the complete event is read and parsed, which should logically lead to two equals copies.

vanmeeuwen requested changes to this revision.May 4 2016, 4:10 PM
vanmeeuwen added a reviewer: vanmeeuwen.

This doesn't really have a "pending approval" or "require answers to questions" or somesuch state, so I'm going to have to make this re-appear on your radar by requesting changes.

This revision now requires changes to proceed.May 4 2016, 4:10 PM
machniak added inline comments.May 5 2016, 1:53 PM
pykolab/xml/event.py
989

Yes, it does. According to icalendar code, but I'll add additional check.

1367

RDATE specified additional occurrences. So if there's one RDATE it is recurring event. There can be an event with RDATE but no recurrence rule. So, I think this is correct. I don't get the last paragraph. get_recurrence_dates() also uses self.event.

machniak updated this revision to Diff 333.May 5 2016, 1:54 PM
machniak edited edge metadata.

Added check for TZID and set tzid variable to None, before using them

vanmeeuwen accepted this revision.May 6 2016, 2:19 PM
vanmeeuwen edited edge metadata.
This revision is now accepted and ready to land.May 6 2016, 2:19 PM
This revision was automatically updated to reflect the committed changes.