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

Support for recurring tasks? #97

Closed
2 of 3 tasks
tobixen opened this issue Nov 1, 2022 · 16 comments
Closed
2 of 3 tasks

Support for recurring tasks? #97

tobixen opened this issue Nov 1, 2022 · 16 comments

Comments

@tobixen
Copy link
Collaborator

tobixen commented Nov 1, 2022

Problem description

In python-caldav/caldav#222 we're trying to use this module for expanding events and tasks with a RRULE into a recurrence set. It works fine for events, but not for tasks. Arguably, expansion of tasks may be a bit exotic and probably not as useful as expansion of events, but tasks aren't that different from events, so it should be relatively trivial to fix.

Futher down in the description, we find that JOURNALs also may have RRULE. Alarms does not have it - ref https://www.kanzaki.com/docs/ical/valarm.html - alarms may have a REPEAT property, but it is not subject for expanding in the same way.

(Came to think, there are several servers out there that supports expansion of events, but not tasks - perhaps some of them are utilizing this python module?)

Reproduction

foo is the icalendar file from below

>>> from datetime import datetime
>>> import recurring_ical_events
>>> import icalendar
>>> recurrings = recurring_ical_events.of(icalendar.Calendar().from_ical(foo)).between(datetime(1991,1,1),datetime(2000,1,1))
>>> recurrings
[]

ICS file

BEGIN:VCALENDAR
VERSION:2.0
PRODID:-//Example Corp.//CalDAV Client//EN
BEGIN:VTODO
UID:19920901T130000Z-123408@host.com
DTSTAMP:19920901T130000Z
DTSTART:19920415T133000Z
DUE:19920516T045959Z
SUMMARY:Yearly Income Tax Preparation
RRULE:FREQ=YEARLY
CLASS:CONFIDENTIAL
CATEGORIES:FAMILY,FINANCE
PRIORITY:1
END:VTODO
END:VCALENDAR

Expected behavior

Roughly the same is expected from a recurring VTODO as from a recurring VEVENT:

>>> recurrings = recurring_ical_events.of(icalendar.Calendar().from_ical(foo.replace('VTODO', 'VEVENT').replace('DUE', 'DTEND'))).between(datetime(1991,1,1),datetime(2000,1,1))
>>> len(recurrings)
8
  • add a test
  • implement the test
  • fix the test
@niccokunzmann
Copy link
Owner

niccokunzmann commented Nov 2, 2022

Hi @tobixen, I have seen your username before somewhere... :)
So, it is a missing feature: I did not have the use-case and rather than over-engineer towards something that people would not like to use this way, I was by principle waiting for you to write this exact issue. (Not that I new that it would happen).

Could you copy the structure from the bug report template into the issue description? It will tell you how to go about implementation.

I am sure you will find the one place where the code is doing the check for the event. I cannot remember having tested that Tasks should be absent so any new test will probably allow you to add a test that tasks are returned. If you want to make 100% sure that tasks receive the same compatibility as the events, you can run all the tests on tasks, too ... Here is the starting point:

class ZoneInfoCalendars(ICSCalendars):

What do you think?


Side Note: I will support you in my free time. If you want this done properly and asap, you can pay me.


BTW: I looked at the linked issue and if you want a caldav object as input to this library, I think other users will value that. Sounds interesting to do. But that is another issue... Also, some of the points you talk about in your implementation are what this library is built for.


Do you need VJOURNAL as well? https://github.com/python-caldav/caldav/pull/222/files#diff-491a5dd4eece9b7ffe85041fb307fce0c648d69c707470fd697dbe2960a9ad7eR1059


I like your library. Since it is GPL licensed, here is the line you want to change:

for event in calendar.walk("VEVENT"):


I tend to release fast. If you commit clear, tested code, I am happy to merge. Do not forget to do the dependecy with >= ... in the requirements.txt

@tobixen
Copy link
Collaborator Author

tobixen commented Nov 2, 2022

Could you copy the structure from the bug report template into the issue description?

I'll try to get it done, but things may go a bit slow fro my side; I'm very busy this week.

Side Note: I will support you in my free time. If you want this done properly and asap, you can pay me.

This is a hobby project for me too, there is currently no other use-case than "get the test code to pass", so unfortunately there is no money in it at this point. :-)

Do you need VJOURNAL as well?

Oh - I wasn't aware that RRULE is a valid property in a VJOURNAL entry, but it is. Yes, definitively it's needed to support recurring VJOURNALs too.

@niccokunzmann
Copy link
Owner

niccokunzmann commented Nov 2, 2022

I had a thought: It just needs a valid example for each one component type. That should be in the RFC.

  • task
  • todo
  • journal
  • alarm
  • allow others

I think, we can pass a selector to choose what component type we want. That might be easiest. Probably to the of function.

@tobixen
Copy link
Collaborator Author

tobixen commented Nov 2, 2022

Timezone also has RRULE. I haven't considered ALARMs with RRULE yet.

There is a small catch, VJOURNAL does not support DTEND, and VTODO has a DUE instead of a DTEND. This means that the DUE needs to be considered when expanding VTODO.

I don't think there are any "official" examples of tasks and journals with RRULE in the RFC, but I haven't searched for it. I'm using this, which is a slightly edited example from the RFC:

BEGIN:VCALENDAR
VERSION:2.0
PRODID:-//Example Corp.//CalDAV Client//EN
BEGIN:VTODO
UID:19920901T130000Z-123408@host.com
DTSTAMP:19920901T130000Z
DTSTART:19920415T133000Z
DUE:19920516T045959Z
SUMMARY:Yearly Income Tax Preparation
RRULE:FREQ=YEARLY
CLASS:CONFIDENTIAL
CATEGORIES:FAMILY,FINANCE
PRIORITY:1
END:VTODO
END:VCALENDAR

@tobixen
Copy link
Collaborator Author

tobixen commented Nov 2, 2022

  • alarm

Alarms may have a REPEAT and DURATION, but no RRULE. (If I've understood it correct, DURATION is the interval between each time the alarm is invoced)

tobixen added a commit to tobixen/python-recurring-ical-events that referenced this issue Nov 2, 2022
@tobixen
Copy link
Collaborator Author

tobixen commented Nov 2, 2022

I am sure you will find the one place where the code is doing the check for the event. I cannot remember having tested that Tasks should be absent so any new test will probably allow you to add a test that tasks are returned. If you want to make 100% sure that tasks receive the same compatibility as the events, you can run all the tests on tasks, too ... Here is the starting point:

Sorry, I just added a simple test for a simple todo. It's probably not so much point to duplicate all the tests for tasks and journals ... there is a cost to it, as I would have to copy and edit lots of ICS files also then I suppose, and execution time of the tests would double. As of the benefits, if all the corner cases are covered for events, then probably they would cover the same corner cases for tasks as well. Tasks may have different corner cases - in particular, the RRULE goes towards the DTSTART, but tasks frequently has a DUE and no DTSTART set.

Getting some sort of support for recurring tasks is also much more important than to get it 100% correct. I've done some checks on how the caldav servers handles recurring tasks and all kind of corner cases - they all do different things. I think it's very important that a task manager would support recurring tasks (often "chores"), but being able to do a search and expand operation for recurring tasks half a year in the future is most often pretty moot. One of the big differences between tasks and events is that for events timing is everything, tasks doesn't need to be done at an exact time (and in practice, quite many of the "chores" are interval-based. Like, washing the floors at home - maybe it's a weekly task, it should typically be done if it's around seven days plus/minus some few since last time it was done. In such a case, projecting the exact time the floors should be washed next year is a pretty pointless task.

@niccokunzmann
Copy link
Owner

Cool! Can you create a pull request?

@niccokunzmann
Copy link
Owner

niccokunzmann commented Nov 3, 2022

I would not worry too much about the test time as we can mark tests and switch them on and off.

Tasks may have different corner cases - in particular, the RRULE goes towards the DTSTART, but tasks frequently has a DUE and no DTSTART set.

That is nice to cover.

copy and edit lots of ICS files

No. But as there are different edge cases, it does not make sense to pursue this idea for now, not for a prototype any way. If there is a prototype, I can ship it, you can use it and we go into a maintain-improve cycle with everyone using the module.

@tobixen
Copy link
Collaborator Author

tobixen commented Nov 3, 2022

Cool! Can you create a pull request?

A pull request containing only a broken test? I thought it would be better to make a pull request combining the test with the necessary code changes to make the tests pass ... but of course, if you want it I will make it :-)

@tobixen
Copy link
Collaborator Author

tobixen commented Nov 3, 2022

[Tasks with due but no dtstart] is nice to cover.

Ok, I'll fix that too.

@niccokunzmann
Copy link
Owner

but of course, if you want it I will make it :-)

Yes, because it makes the changes transparent, the CI runs and people can chip in code if they like to more easily, see everything in one place discussed instead of in the commits. the commits do not have to reference this to be found.

tobixen added a commit to tobixen/python-recurring-ical-events that referenced this issue Nov 3, 2022
@niccokunzmann niccokunzmann mentioned this issue Nov 4, 2022
@niccokunzmann
Copy link
Owner

niccokunzmann commented Nov 10, 2022 via email

@niccokunzmann
Copy link
Owner

niccokunzmann commented Nov 16, 2022

Release version 1.1.0b should tackle this feature. #100.

Please let me know if you consider this issue as closed or what else you think would be necessary to close it.

Very likely, issues with edge cases of the current implementation will come up in your case. My proposal is that they go into a new issue.

Another invitation: Would you like to co-maintain this project with me?
Reason:

  • I do not use the TODOs or journals, so I am not knowledgeable about this - you are.
  • You did well testing
  • We were able to talk and you seem to understand how to contribute to the ecosystem in other places
  • You have a reason to keep this project going in high quality
  • I am the sole maintainer - danger zone!
  • You might not want to depend on a guy at the other end of the world living in [locally relative] poverty - just not reliable enough!

What do you think? Happy to have you - I can still throw you out, so no problem! 😄

@tobixen
Copy link
Collaborator Author

tobixen commented Nov 17, 2022

I'm unable to commit myself to anything more than what I'm already committed for, and I'm not sure I will be able to do much work with this one over the next few years (only sheer luck that I've had the opportunity to spend significant time doing open source hacking over the last few months). I don't want to end up like I did with the caldav library (actually I thought I wouldn't have to learn anything about the caldav protocol if I used a high-level library for dealing with it ... but eventually the caldav library was buggy and incomplete, so I got my hands dirty ... the rest is history).

However, the fifth thing on your list is an important one. Any project should have redundancy in the maintainership. I remember it was a great issue some few years ago with the vobject library - the maintainer had abandoned the project, it didn't work under python3 and had some other bugs and issues, there existed several forks of the vobject library, but it was a mess. I don't know how it ended, but I guess the original maintainer eventually showed up and gave away the project to someone else. From this point of view, I guess there is some value in having a co-maintainer even if said person is relatively inactive.

tobixen added a commit to python-caldav/caldav that referenced this issue Nov 17, 2022
@tobixen tobixen closed this as completed Nov 17, 2022
@tobixen
Copy link
Collaborator Author

tobixen commented Nov 17, 2022

Oh, the checkboxes confused me a bit. "add a test", "implement test", "fix test" ...

I assume I understand "add a test". That's to create test code that will reproduce the issue and report a failure or an error.

I also assume I understand "fix the test", I assume that means "fix the bug and ensure the test code passes".

But what then is "implement the test"?

niccokunzmann added a commit that referenced this issue Nov 19, 2022
@niccokunzmann
Copy link
Owner

niccokunzmann commented Nov 19, 2022 via email

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

No branches or pull requests

2 participants