-
Notifications
You must be signed in to change notification settings - Fork 1
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
repeatFrequency must be accompanied by appropriate byDay/byMonth/byMonthDay values #362
base: master
Are you sure you want to change the base?
Conversation
…yDay/byWeek/byMonthDay values
Noting that this came up in a recent conversation which noted a bug in the spec: openactive/modelling-opportunity-data#260, which impacts (3) above. Though it seems to already have been taken into account in the code of this PR (even if it's not obvious from the description of it)! |
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.
Looks great! Mone minor suggestion
|
||
|
||
/* | ||
Ensures that the value given in repeatFrequency is matched by an appropriate byDay, byWeek, or byMonth attribute. Possible values for repeatFrequency are in the first instance P[\d]D, P[\d]W, P[\d]M. In the first instance the repeatFrequency should be paired with a byDay attribute, in the second, byWeek, etc. Note that this test checks for the bare presence of appropriate attributes; there is no semantic checking |
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.
Is P[\d]Y
also possible?
} | ||
// check frequency is valud | ||
repeatFrequency = repeatFrequency.toLowerCase(); | ||
const regexp = /^p\d(d|w|m)$/; |
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.
should this allow annual repeats?
The rule that monthly schedules must have |
Good catch @lukehesluke ! Feel free to request further changes! |
@thill-odi I've suggested a possible set of requirements here: openactive/modelling-opportunity-data#260 (comment). Let me know if that looks good or needs tweaking |
Ensures that:
repeatFrequency
is specifiedrepeatFrequency
is well-formed according to a limited subset of ISO 8601 duration valuesrepeatFrequency
value is accompanied by abyDay
,byMonth
, orbyMonthDay
values.