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

Introduce EvaluationOptions for params like MaxIncrementCount #747

Open
minichma opened this issue Mar 5, 2025 · 4 comments · May be fixed by #750
Open

Introduce EvaluationOptions for params like MaxIncrementCount #747

minichma opened this issue Mar 5, 2025 · 4 comments · May be fixed by #750

Comments

@minichma
Copy link
Collaborator

minichma commented Mar 5, 2025

Today we the param MaxIncrementCount is hard coded in RecurrencePatternEvaluator. It limits the number of iterations that will be calculated by the class without returning any recurrence, before aborting the calculations.

The implementation has two major shortcomings:

  1. It is not configurable
  2. The calculation aborts silently if the limit is hit.

Ad 1)
It would be good to introduce something like EvaluationOptions that is passed to Calendar.GetOccurrences et al. For now the class should hold only the MaxIncrementCount param that should replace the hard-coded value. In the future it could be extended by additional params. The new param to GetOccurrences() would default to null, in which case default values would apply. The default value is TBD, I would suggest to have no limit as the default.

Ad 2)
If the limit is hit, calculation stops silently, which prevents users from detecting the case. The default behavior should be that a specific exception is raised, maybe something like IcalEvaluationLimitExceededException or alternatively some more generic IcalEvaluationException holding a more detailed error code as a property. Basically the behavior (fail/stop silently) could also be configured via EvaluationOptions but that doesn't seem to be too helpful as users can easily catch the exception and implement ignore it at the call site if desired.

In the future the EvaluationOptions could hold additional properties

  • the TZDB to be used
  • ...

I think it would be good to introduce the EvaluationOptions still in v5 so we have an extension point that allows us adding more in the future, without breaking the API.

@axunonb
Copy link
Collaborator

axunonb commented Mar 5, 2025

Fully agree, see #641, this is is a "must".
(BTW we also have the NRT on the agenda for v5)

@minichma
Copy link
Collaborator Author

minichma commented Mar 5, 2025

Sorry, missed #641. Close this one as duplicate?

NRT would certainly be nice but activating it later would probably not be a breaking change. Would it?

@axunonb
Copy link
Collaborator

axunonb commented Mar 5, 2025

Sorry, missed #641. Close this one as duplicate?

This one is more descriptive. I closed the old one.

NRT would certainly be nice but activating it later would probably not be a breaking change. Would it?

Yes, introducing NRT means a breaking change.

@minichma
Copy link
Collaborator Author

minichma commented Mar 6, 2025

@axunonb Just to avoid double work: I started working on this. Will try to draft a PR later this week.

@minichma minichma linked a pull request Mar 6, 2025 that will close this issue
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 a pull request may close this issue.

2 participants