nusmodifications / nusmods

🏫 Official course planning platform for National University of Singapore.
https://nusmods.com
MIT License
578 stars 315 forks source link

use RRULE INTERVAL instead of EXDATE #383

Closed jethrokuan closed 7 years ago

jethrokuan commented 7 years ago

Currently the ICS export uses EXDATE for biweekly tutorials/lectures, which is less compliant to the iCalendar spec of setting the RRULE to FREQ:WEEKLY;INTERVAL:2;COUNT=7. I'm not sure how ICS exports works underneath as of yet, but this seems simpler.

jethrokuan commented 7 years ago

Here's where the relevant changes need to be made in v3: https://github.com/nusmodifications/nusmods/blob/38061211f0517df29ac2eefd85ba2323f0cde2cc/v3/src/js/utils/ical.js#L86-L92

jethrokuan commented 7 years ago

note that I wouldn't mind making the changes, but I'm not sure exactly where I should be contributing: v3...?

ngzhian commented 7 years ago

@jethrokuan so v3 has ical implementation which uses rrule I believe (it uses ical-generator under the hood), relevant code is https://github.com/nusmodifications/nusmods/blob/master/v3/src/js/utils/ical.js#L134.

So if you would like to make sure v3 uses rrule, please do it in v3. If you would like to change current nusmods, I think we eventually want to move to v3, so that's not that high a priority.

jethrokuan commented 7 years ago

@ngzhian I see that, but it doesn't use INTERVAL and consistently uses NUM_WEEKS_IN_A_SEM as the COUNT. Lines 86 to 92 of the same file shows it uses EXDATE. Should be a relatively simple change, but I'm not sure what the process of validating PRs are.

ngzhian commented 7 years ago

Ah i see what you mean @jethrokuan , I think I chose to implement this way because even weeks aren't really interval of 2 here. nus weeks look like this: 1, 2, 3, 4, 5, 6, RECESS, 7, 8, 9 ... an odd week lecture will be on: 1, 3, 5, 7, 9 which is interval 2 from 1-5, then it actually has an interval of 3, and that's why I chose weekly with excludes. If you have an idea how to do this, do open a PR and ref this issue and we will take a look :)

jethrokuan commented 7 years ago

oh yeah, I guess then that implementation is fair, didn't think of that. My use case is integration with another calendar application, but there wouldn't be a clean implementation anyway.

ngzhian commented 7 years ago

@jethrokuan another way is that you can return 2 events that make up this, 1 pre recess week, 1 post recess week. But there are problems with this as well, if you want to change the name then you have to change it twice (since those are disjoint events now). Can try using the same ID for both events, but I'm not sure if that will be allowed. Anyway feel free to experiment with this! Lmk if you find anything interesting / have questions 🍻

jethrokuan commented 7 years ago

I thought about that, in particular the fact that the timetable events don't really change makes it a viable solution. That would certainly solve my issue, but I don't think it's a much better solution than EXDATE for the general public.