-
Notifications
You must be signed in to change notification settings - Fork 91
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
Expand recurring events client-side #222
Expand recurring events client-side #222
Conversation
caldav/objects.py
Outdated
:param event: Event | ||
""" | ||
import icalendar | ||
import recurring_ical_events |
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.
We already have a dependency on dateutil.rrule
. If it's easy to rewrite this code to also use dateutil.rrule
, or if it's easy to rewrite my code to use recurring_ical_events
, then I think it would be nice to reduce the number of dependencies. If not, I think we can move this import also to the top of the file, and add it into setup.py
.
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.
Tests also break because the dependency is not mirrored in setup.py
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.
If it's easy to rewrite this code to also use
dateutil.rrule
, or if it's easy to rewrite my code to userecurring_ical_events
Ok I'll look into it.
I've added some "critical" comments as direct comments towards the code, but don't get me wrong - this is cool, and I want to merge it. Some general comments:
|
Thanks for your review!
Ok I'll look into it.
I'm not sure... what do the various RFCs have to say about this? I didn't look honestly.
Sure! As I said this is really a PoC. I have a couple of questions for you:
|
The expand logic should only return one I'm now working on a |
On the icalendar level, recurrence instances should inherit most of the data from the parent, but RRULE should be removed and a |
In the expanded set of recurrences, none of the VEVENTs should have RRULE set. The RFC is very clear on that. |
See https://www.rfc-editor.org/rfc/rfc4791#section-7.8.3 and https://www.rfc-editor.org/rfc/rfc4791#section-9.6.5 for information on how the expanded icalendar data should look.
|
I wasn't considering the Todo objects by the way...
I'm talking about the non-VCALENDAR attributes such as client, id, URL, parent, coming from DAVObject. Or am I getting it wrong?
Wait, are we working on the same thing? Should I stop? :-) |
It is not particular normal, and perhaps not even particularly useful to expand recurring TODOs, but it's still important to consider them :-)
You should not be initiating multiple
I'm not doing any expansion in my code, I am only doing the splitting/copying from one Event-objects with a recurrence set in the data, into multiple Event-objects with one recurrence in each of them. So your code should not generate new Event-objects. |
My code is in #223, though I've managed to mix up my commits a bit, so it's mixed together with some bugfixes now. I'll try to split it up in two changesets. |
So far I've addressed the following (almost) successfully:
However, I said almost because
There is some discussion on the matter:
I'll look further into it. In the meantime you might want to try your code in #223 against mine. |
If you get it to work with the recurring_ical_events library, but not with the dateutil.rrule, then it's no point spending too much time arguing with dateutil.rrule. |
I have the branch I will write up some test code before merging #225 to master, though came to think I can use your function for creating test data for testing my function :-) |
I see the test code fails now, because the default value for expand is
I also think it should be made into a method under the CalendarResourceObject class rather than a function. I can take over if you want, but to get the annotations right it's better you do it :-) |
Remove recurrance properties and VTIMEZONE from occurance events; Use dtstart as recurrence-id; Create one VCALENDAR with multiple VEVENTS as result
Surely there must be a better way to do this though.
Ok I've rebased my master branch onto #225. Before I follow up on your deprecation notice on
So I'm gonna go ahead on your advice and go back to using |
I'm trying to move the expansion logic in |
Not quite finished yet though.
Started writing test code. The bad thing with expanding the object in-line is that information is lost, so the object needs to be initiated for each test that is done. Anyway ... the expanded data should contain the RECURRENCE-ID header. I think it should be equal to DTSTART. |
|
(the above is from the 222-branch. You may pull in the changes from that branch eventually) |
Thanks. I committed some minor mistakes that I see you pulled. Something is not working right though. I'm checking now. |
Thanks. I committed some minor mistakes that I see you pulled. Something is not
working right though. I'm checking now.
Except for the recurrence-id, the expand logic seems to work fine.
…--
Tobias Brox
Senior System Consultant
Redpill Linpro AS - Changing the game
Mobil: +47 917 000 50
Kontor: +47 215 441 68
|
Ok sorry it was my fault, it all seems to work now. The tests passed too after applying 506a282. What's left (in objects.py): # TODO remove or downgrade to debug
logging.info( I'd remove it altogether, little to no added value. # FIXME too much copying
stripped_event = self.copy(keep_uid=True) Surely there is a better way to do this. Then I'll begin squash some of the commits - history is too dirty for merging. |
Yes, everything should be squashed, but it can wait until all the tests pass. Check my last commit in the 222-branch :-) |
Regarding instance vs object vs ...
Internally the icalendar data is always present as either raw data, an icalendar object or a vobject object. Whenever one of those "magic" properties are used, if other representations exists, it will be converted and the old representation will be thrown away. Now there is a slightly bad thing with this ... such properties are not supposed to have side effects, but details like ordering of the properties and line wrapping may be changed when accessing the properties. I also see that there may be a potential for bugs there. |
|
Three failing tests now, and they are all related to the module not supporting tasks. I will consider making work-arounds in the test code unless this can be fixed in the module relatively fast. The logging ... yes, probably better to just remove it. As for the copying - I will think a bit if I can come up with a better idea, but the overhead is not too bad anyway. |
|
I was going to open an issue to I'm here if you need help with something - I might get busy too in the following days but I can usually act within 24 hours or so. |
I sort of started to write up a test code for tasks in the python-recurring-ical-events project, but got interrupted with it. I'll get it done now |
As you can see, I wrote up a test for the python-recurring-ical-events project, but I don't have time writing up the code change now. If you beat me at it you may give it a go :-) |
Let's see ... if I remember right the only reason why this isn't merged is that the testTodoDatesearch does not pass, and the reason for that is that the recurring-ical-events library does not support todos. The code is in place in a pull request there now, but in the meanwhile another test on that project broke after the latest release of a new icalendar library. The author of the recurring-ical-events library has written up a pull request for the icalendar library ... but ... it's like a never-ending rabbit hole. I think I'll just make some temporary workaround and skip the broken tests as for now so that v0.10.1 can be released. |
I squashed some of the commits, pulled it into master, polished a bit and put code in place for skipping two broken tests. Will try to get v0.10.1 released (just need to run tests towards a lot of different servers first) |
Thank you! I'll begin working on the Home Assistant side to use this. |
Hi there,
I've been trying to implement a fix for #157. I've been testing this with my Home Assistant installation (ref. home-assistant/core#40127) and it seems to be working, but I don't know if I'm doing this right (I've worked on this for like 3 hours).
A few challenges I had:
caldav.Event(data=icalendar_Calendar_object.to_ical()
Please be aware that this is PoC-quality code: it's meant as a way to address this issue and start having feedback. I will rewrite it from scratch when everyone is confident that it could work - I will leave the draft status until that.
Before going deeper into this (e.g. writing tests) I'd like to have some feedback please.
This patch needs the following packages installed: