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

Fix previous_months_iterator #358

Merged
merged 5 commits into from
Jun 10, 2021
Merged

Conversation

bryanlandia
Copy link
Contributor

Addresses #356

Avoid rrule when possibly iterating forward from days 29-31 of months,
rrule simply skip month if it tries to iterate to e.g., February 30.

@bryanlandia
Copy link
Contributor Author

Hi @johnbaldwin I've updated the helper method and associated test method. The previous_months_iterator is now working properly across short months, leap years, and returns the correct number of months back.

Avoid rrule when possibly iterating forward from days 29-31 of months,
rrule simply skip month if it tries to iterate to e.g., February 30.

previous_months_iterator start num_months -1 back to incl. current

Since changing the method of providing a previous months generator, we
need to start one fewer months back or we lose the current month in
the output.  current_month should be last month returned in history
frontend code expects last entry in history to be current_month:
for example, in code for current month change since last month,
comparison is to `this.state.valueHistory.size-2`
-1 since array is 0-indexed, -2 to find previous month val in history

Probably the months_back 7 was being used in tests because of the
issue that this PR addresses.
If month_for is June and months_back is 6, should return last day of
June and 5 previous months--Jan, Feb, Mar, Apr, May, June
Handle months_back of zero properly, returning the month_for month
@bryanlandia bryanlandia force-pushed the bryan/fix-prev-months-iterator branch from d0c4b13 to eaf2455 Compare June 10, 2021 01:31
Copy link
Contributor

@johnbaldwin johnbaldwin left a comment

Choose a reason for hiding this comment

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

@bryanlandia Looks reasonable to me. Thanks!

@bryanlandia bryanlandia merged commit d4e7847 into master Jun 10, 2021
@OmarIthawi OmarIthawi deleted the bryan/fix-prev-months-iterator branch March 19, 2022 11:09
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.

2 participants