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

[AERIE-1911] - Range check recurrence interval #232

Merged

Conversation

pranav-super
Copy link
Contributor

@pranav-super pranav-super commented Jun 22, 2022

  • Tickets addressed: AERIE-1911
  • Review: By commit
  • Merge strategy: Merge (no squash)

Description

The focus of the PR is to range check recurrence intervals, that is, not allow negative recurrence intervals. To do this, we catch the error within the builder of the RecurrenceGoal (specifically in fill()). Also, since the RecurrenceGoal stores its interval as a window (with the start representing a minimum recurrence interval, the end representing a maximum, a feature not yet supported), that was replaced with a record MinMaxAllowableRecurrenceInterval just to make it more explicit.

Verification

A new test was added to the existing recurrence goal tests - TestRecurrenceGoal.
Similarly, another test was added to the integration tests, to verify the entire process worked.

Documentation

Just an update to the recurrence goal section.

Future work

This might play weird with the AnalyzeWhen ([AERIE-1880]) PR, just because of the MinMaxAllowableRecurrenceInterval fix.

Copy link
Contributor

@patkenneally patkenneally left a comment

Choose a reason for hiding this comment

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

🔻 Issues to address before merge
🔶 Requests that should not block merge, but should at least be discussed
🔵 Recommendations that can be ignored if desired

@pranav-super pranav-super force-pushed the feature/AERIE-1911--range-check-recurrence-interval branch 3 times, most recently from 6167879 to 03acf09 Compare June 28, 2022 20:23
@pranav-super pranav-super force-pushed the feature/AERIE-1911--range-check-recurrence-interval branch 2 times, most recently from 8dabd66 to fb59945 Compare July 5, 2022 21:30
@patkenneally patkenneally self-requested a review July 7, 2022 17:41
@pranav-super pranav-super force-pushed the feature/AERIE-1911--range-check-recurrence-interval branch from fb59945 to 579708b Compare July 7, 2022 17:48
@pranav-super pranav-super merged commit 4be0422 into develop Jul 7, 2022
@pranav-super pranav-super deleted the feature/AERIE-1911--range-check-recurrence-interval branch July 7, 2022 18:03
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.

6 participants