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

Add isFinite, first & last to rrule & rruleset #422

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ephys
Copy link

@ephys ephys commented Sep 28, 2020

This implements feature request #215

  • isFinite returns true if the rrule/rruleset has a determinable final occurrence
  • last returns the final occurrence if it exists and is determinable
  • first returns the first occurrence if it exists

This PR also implements the following revertible changes:

  • calling count on an infinite rrule/rruleset now returns Number.POSITIVE_INFINITY instead of returning all possible dates until the year 9999 - could be considered breaking.
  • calling all on an infinite rrule/rruleset now throws instead of returning all dates until the year 9999 (unless an iterator has been provided as the iterator can stop the loop) - could be considered breaking.
  • options.count throws if it received something other than a safe integer, null or Number.POSITIVE_INFINITY. Number.POSITIVE_INFINITY normalizes to null.

Thanks for contributing to rrule!

To submit a pull request, please verify that you have done the following:

  • Merged in or rebased on the latest master commit
  • Linked to an existing bug or issue describing the bug or feature you're
    addressing
  • Written one or more tests showing that your change works as advertised

@KamalAman
Copy link

This would be a lovely PR to add if it is still valid.

@dereekb
Copy link
Contributor

dereekb commented Jul 6, 2021

Was just looking to add this functionality to my project, specifically checking for finiteness and finding the last date.

return this.before(this.options.until, true)
}

const allDates = this.all()
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion to use this.before() instead of this.all() here with a max date (new Date(9999, 1, 1), or some other date that is large enough to be agreed upon) so that the values from .all() are not all stored into memory while computing the result.

Copy link
Author

Choose a reason for hiding this comment

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

Date has a max value which seems to be new Date(8640000000000000);, anything after that returns "Invalid Date". We could use that if it's not more expensive (which I think it isn't)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea, that or a similar value works.

Technically the Iter loop won't works/exits properly with that as the MaxDate in IterResult, since technically tooLate will never be set to true, as new Date(8640000000000001) > new Date(8640000000000000) is false.

Would just round the numbers and go with new Date(8000000000000000); to avoid that issue. Kind of doubt people are using dates that are in the year 255000+.

That said, the Javascript VM is probably going to long before it gets there if for whatever reason the iteration is that large. Would leave it up to code outside of the library to catch malicious/bad input that would let a massive amount of iterations to occur.

Copy link
Author

Choose a reason for hiding this comment

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

Ah yes you're right. But I think it could be a good occasion to fix the exit condition to take NaN dates into account! :)

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.

3 participants