Improve recurrence handling, remove restrictions #627
Conversation
- Deprecated `RecurrenceRestrictionType` and `RecurrenceEvaluationModeType` enums. Updated RecurrencePattern.cs: - Deprecated `RestrictionType` and `EvaluationMode` properties. - Set default for `RestrictionType`. Enhanced RecurrenceUtil: - Added new using directives. - Introduced `DefaultTimeout`. - Updated `GetOccurrences` with timeout logic. Refined RecurrencePatternEvaluator: - Removed `_maxIncrementCount` with connected logic in favor of `RecurrenceUtil.DefaultTimeout` - Improved XML documentation comments. Updated Calendar.cs: - Simplified `Load` method. - Deprecated certain properties and methods associated with `RestrictionType` and `EvaluationMode` - Removed obsolete `Evaluate` method. Enhanced RecurrenceTests.cs with more robust test cases: - Replaced `Assert.That` with `Assert.Multiple`. - Changed assertion for occurrences count. - Renamed test methods for clarity. - Added timeout settings and updated expected exceptions. - Adjusted test data and added comments. Corrected typos and improved formatting for better readability. Fixes ical-org#622
|
@minichma |
|
@axunonb no worries 😄 Didn't have time to go into detail so far, just my two cents on timeouts in general. I think it would be a good idea to keep the behavior of the algorithms deterministic. By introducing a timeout, we lose determinism, which can cause quite some issues of different kind. E.g. debugging is getting more challenging, as the timeout will expire quickly after a breakpoint was hit. More importantly the libs will behave differently depending on the system they are running on and the load that system currently experiences. So on server systems people might start seeing exceptions at peak hours in code that normally runs just fine. So all in all I would rather discourage the implementation of time-based behavior. I think the limitation based on increments was not too bad, because its fully deterministic, but certainly should be configurable. An alternative could be to allow users to configure a callback that's invoked frequently (probably on every increment). Users can then implement a timeout on their own. Or maybe pass a CancellationToken to the evaluation function that allows the caller to cancel based on a timeout, though this the callback would be preferable, as the CancellationToken wouldn't allow implementing deterministic behavior either. |
|
Thanks for the quick feedback! "Deterministic algorithms" is a valid point, agree.
Really hard to decide. What would you prefer? |
This should be implemented anyways, because there's really no need (from a user's perspective) to iterate over all occurrences if just a few are needed. And more importantly, AFAIK the order in which HashSet returns items is not defined, so the user would have to sort them before using. But this would just be an addition, not an alternative to 1/2, right? Because if no occurrences would be returned, it could still run forever. From the other two options I would probably prefer 1, because its very straightforward. Question would be, how to do the configuration. I'd suggest to pass something like That GetOccurrences could block for an indefinite amount of time is not something I see critical. To some degree we could say, this is how computers work. The time processing takes is most often dependent on the CPU and system load, so this is not something special here. Its not on us to decide how long a task is supposed to take. And generally the duration should be low. If its noticable, there's something wrong anyways. One other thought I have is whether it should be possible that the iteration runs for a noticable amount of time at all. It should probably be possible to simplify the whole algorithm in a way, that it will only process time values that will eventually be returned to the user (exception: very large |
|
I'm fine going this way, thanks. |
- Removed `SetupBeforeEachTest` and timeout settings from tests. - Re-introduced `_maxIncrementCount` in `RecurrencePatternEvaluator`. - Modified `GetDates` to use `_maxIncrementCount` for loop control. - Removed timeout handling from `RecurrenceUtil` class.
axunonb
changed the title
- Updated test method names to reflect checking for at least a defined number of occurrences. - Removed redundant summary comments to test methods for clarity. - Remove remaining mentions of "Timeout"
|
@minichma
What if we merged it and published a new minor version? |
* Secondly_DefinedNumberOfOccurrences_ShouldSucceed(), Minutely_DefinedNumberOfOccurrences_ShouldSucceed(), Hourly_DefinedNumberOfOccurrences_ShouldSucceed() assert exact number of occurences. * Updated test methods to dynamically generate expected occurrences using loops
|
@minichma Do you think this PR needs some more tasks to be completed? Otherwise I'd like to proceed as described. |
I think it's good as is. Left just a few minor comments. |
|
RecurrenceRestrictionTypeandRecurrenceEvaluationModeTypeenums.Updated RecurrencePattern.cs:
RestrictionTypeandEvaluationModeproperties.RestrictionType.Fix RecurrenceUtil:
GetOccurrences()overloads convertIDateTimearguments withAsSystemLocal. This partial conversion is considered a bug. The change may break user code that contains a fix for the bug.Refined RecurrencePatternEvaluator:
Updated Calendar.cs:
RestrictionTypeandEvaluationModeGetOccurrences()overloads convertIDateTimearguments withAsSystemLocal. This partial conversion is considered a bug. The change may break user code that contains a fix for the bug.Evaluatemethod (just threw when called)Enhanced RecurrenceTests.cs with more robust test cases:
Assert.ThatwithAssert.Multiple.Corrected typos and improved formatting for better readability.
Fixes #622