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

feat: support adding and fetching recurrence with rrule #128

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

differental
Copy link

Adds support of adding and fetching recurrence with rrule, which follows RFC-5545. Tests are added as well.

Note: Presumably src/period.rs and src/repeats.rs can be removed if this is a satisfactory implementation.

@@ -356,6 +357,27 @@ pub trait EventLike: Component {
self.property_value("LOCATION")
}

/// Set recurrence rules
fn recurrence(&mut self, rruleset: RRuleSet) -> &mut Self {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this ignores any EXRULE, RDATE or EXDATE from the set, which seems misleading. I think it should either add them as well (and maybe do something about the DTSTART matching?) or take an RRule rather than an RRuleSet.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RDATE and EXDATE added - thanks for spotting this. (EXRULE is deprecated)

For DTSTART matching, I can try adding it. The logic might become a bit complex though, since rrule takes a chrono::DateTime<Tz> and icalendar has a DatePerhapsTime. I would imagine it'll need some methods under chrono-tz flag, so I'm not sure whether it's best to be included in this pr as well... open to your thoughts. ;)

@differental
Copy link
Author

Sorry for the oversight. rrule v0.10.0 is the latest supported by rustc 1.60, hence that was added when I ran cargo add.

v0.10.0 also did not put RDATE, EXRULE and EXDATE in fmt, leading me to believe everything's in RRULE.

I'll update my pr to include these three, hopefully v0.10.0 is sufficient.

@hoodie
Copy link
Owner

hoodie commented Apr 2, 2025

Hi, first of all thanks for the contribution. I've been putting this off thinking we could maybe implement a native/own version of this here at some point, but now that I've looked at the rrule crate, there's a lot of stuff in there that I wouldn't want to debug myself 😆

I welcome this addition, still, it pulls in a large dependency and change a few things. So let's be a bit careful. I would recommend making this an optional feature and making sure that it works together well with the parsing aspects of this library.

@hoodie
Copy link
Owner

hoodie commented Apr 5, 2025

You probably have some nice usecase for rrule. Could you perhaps write one or two examples that illustrate what we can do with it?

@hoodie
Copy link
Owner

hoodie commented Apr 5, 2025

I was just playing with it briefly, I don't have a lot of time so I didn't get a good feel for the api yet. It might be that the API experience is still a bit complicated, when you have to give it a start data with the rrule tz. Also I wonder if an event itself that has a recurrence should maybe implement ToIterator or something?

Here is my short attempt at an example (5min efford).

use chrono::*;
use icalendar::*;

fn main() {
    let my_calendar = Calendar::new()
        .name("example calendar")
        .push(
            // add an all-day event
            Event::new()
                .all_day(NaiveDate::from_ymd_opt(2016, 3, 15).unwrap())
                .recurrence(
                    rrule::RRule::new(rrule::Frequency::Yearly)
                        .build(chrono::Utc::now().with_timezone(&rrule::Tz::UTC))
                        .unwrap(),
                )
                .summary("My Birthday")
                .description("Hey, I'm gonna have a party\nBYOB: Bring your own beer.\nHendrik")
                .done(),
        )
        .done();

    for event in my_calendar
        .components
        .first()
        .unwrap()
        .as_event()
        .unwrap()
        .get_recurrence()
        .iter()
    {
        println!("Event: {:?}", event);
    }

@hoodie
Copy link
Owner

hoodie commented Apr 5, 2025

Would be nice if we could create a birthday event and then .iter().take(5) print the first 5 occurences of the event. or similar. I'm not saying you have to write this example, I'm just dumping my brain :D

@differental
Copy link
Author

The recurrence_parse example isn't working as intended - the example ics in fixtures/icalendar-rb/recurrence.ics seems to not have EXDATE immediately after RRULE and this is causing issues. I'll change the code to parse them separately

@differental
Copy link
Author

Oops sorry didn't see the comments - I added two examples that seem to work, and thankfully the parsing example with the recurrence fixture helped me discover the bug with parsing ics. Had to make some significant changes but now everything should work - I added an extra test just to be sure

@differental
Copy link
Author

Would be nice if we could create a birthday event and then .iter().take(5) print the first 5 occurences of the event. or similar. I'm not saying you have to write this example, I'm just dumping my brain :D

That actually looks cleaner than their "recommended" method .all(limit).dates -> Vec<DateTime>, maybe I should change the one I added to the example ;)

Sorry for messing this up by the way, I'm glad there were some fixture ics that caught my bug

@hoodie
Copy link
Owner

hoodie commented Apr 5, 2025

No worries. Still grateful for the contribution. This is actually a complex feature. Let's take it slow.

@differental
Copy link
Author

It should work now though, I'll double-check the parsing just in case

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.

3 participants