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 Schedule with FREQ=DAILY #260

Open
nickevansuk opened this issue Feb 2, 2021 · 5 comments
Open

Support for Schedule with FREQ=DAILY #260

nickevansuk opened this issue Feb 2, 2021 · 5 comments

Comments

@nickevansuk
Copy link
Contributor

nickevansuk commented Feb 2, 2021

The current spec appears to have a typo:

While marked as optional, publishers also need to provide additional information about the frequency, e.g. whether the event occurs on a specific day, day of the month, or month, by including one of the schema:byDay, schema:byMonth or schema:byMonthDay properties

This should read "publishers may also need to", to support iCal's FREQ=DAILY.

@lukehesluke
Copy link

@nickevansuk should we extend this issue to updating the spec so that it exactly lays out the requirements for byDay / byMonth / byMonthDay properties depending on the value used for repeatFrequency?

With the aim being for the text of the spec to match with the validation logic being implemented in this PR: openactive/data-model-validator#362.

@lukehesluke
Copy link

lukehesluke commented Mar 29, 2021

@nickevansuk actually it looks like the above PR doesn't support the "2nd Thursday of the month" use case (comment).

Perhaps instead we should lay out the requirements like so:

  • repeatFrequency must be a restricted ISO-8601 duration that can only be in one of these case-insensitive forms (take X to be a placeholder for some number of digits e.g. PXD could be used as P1D or P2D or P10D etc):

    • PXD
    • PXW
    • PXM
    • PXY

    Note that compound durations like P1Y2M are not supported

  • byDay items can come in two flavours:

  • For schedules with daily frequency (PXD), byDay, byMonth and byMonthDay MUST all be absent

  • For schedules with weekly frequency (PXW), byDay MUST be present and byMonth & byMonthDay MUST be absent.

    • byDay items MUST be SchemaWeekDays
  • For schedules with monthly frequency (PXM), byMonth MUST be absent and at least one of byMonthDay and byDay MUST be present

    • byDay items MUST be MonthlyICalWeekDays
  • For schedules with annual frequency (PXY), byMonth MUST be present and a least one of byMonthDay and byDay MUST be present

    • byDay items MUST be MonthlyICalWeekDays

@nickevansuk
Copy link
Contributor Author

nickevansuk commented Mar 30, 2021

This looks great @lukehesluke! Two questions:

  1. For MonthlyICalWeekDay, does this need to be an array to map to iCal or can it just be singular? (I’m wondering whether it was made an array unnecessarily in version 2.0 which just adds complexity)

  2. For restricted repeatFrequency, does this happen naturally when mapping from iCal format?

In terms of moving this forward in the spec:

  • Ideally we would define a brief pseudocode conversation (either in code form or using columns in a table with e.g a row for each property) between iCal’s RRule and Schedule that is bidirectional and complete - and that would work in any language. Including this in the spec itself would remove any ambiguity in implementations. Hopefully such a conversation is straightforward.

  • Once such a conversation is defined it should be straightforward to also state any restrictions or invariants implied by it. Such restrictions or invariants can then be rules in the validator.

  • If we document both of the above in this issue, we can ask all the developers who have currently implemented Schedule to review it against their own implementations and check for discrepancies. This should catch anything we’ve missed.

  • From there expanded guidance can easily be written on the developer docs site, and the text of the spec updated.

@lukehesluke
Copy link

@nickevansuk

in answer to your questions:

  1. It could be singular, using iCal's comma-separated format

    ✅ This does provide a simple method for differentiating between SchemaWeekDays and MonthlyICalWeekDays strings (if (Array.isArray(schedule.byDay)) { // is an array of SchemaWeekDays).

    ✅ This would make it (just slightly) easier to integrate with libraries which can parse iCal strings e.g. with rrule, you can use:

    RRule.parseString(`BYDAY=${schedule.byDay}`);
  2. Restricted repeatFrequency does occur naturally when mapping from iCal format as the FREQ string can only have one of these values: "SECONDLY" / "MINUTELY" / "HOURLY" / "DAILY" / "WEEKLY" / "MONTHLY" / "YEARLY" i.e. it cannot be "1 minute and 30 secondsly"

Sticking so closely to the iCal spec does beg the question of why not just use an iCal text rule e.g. a string iCalRecurrenceRule field in the Schedule. Doing this would allow implementers to use existing libraries (like rrule) rather than building new ones.

I wasn't able to find an issue in the past that discussed and decided against using a direct iCal rule. Should this be the direction to go in?

@nickevansuk
Copy link
Contributor Author

nickevansuk commented Mar 30, 2021

  1. Right ok good to know, if it's otherwise comma separated an array is indeed better modelling practice.

  2. Great news, these constraints sound good in that case

From memory the rationale is that an iCal rule is pretty impenetrable to data processors who do not have an iCal library and just want to display "12pm on Tuesday" - the full discussion may well be in a W3C call. For simple schedule rendering (as in e.g. http://visualiser.openactive.io/) understanding of iCal is not required.

This thread contains much of the rationale around the Schedule design: #159 (comment).

So although in practice most data publishers will need to convert iCal to a Schedule to render it, and most data users will need to convert a Schedule to iCal to process it, keeping the Schedule model makes the barrier to entry for data use low for the simple 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

No branches or pull requests

2 participants