Evaluation: Make MaxIncrementCount configurable
#750
Conversation
Codecov ReportAttention: Patch coverage is ❌ Your patch status has failed because the patch coverage (68%) is below the target coverage (80%). You can increase the patch coverage or adjust the target coverage. @@ Coverage Diff @@
## main #750 +/- ##
===================================
Coverage 66% 66%
===================================
Files 105 104 -1
Lines 4631 4611 -20
Branches 1150 1146 -4
===================================
- Hits 3055 3047 -8
+ Misses 1139 1129 -10
+ Partials 437 435 -2
🚀 New features to boost your workflow:
|
|
@axunonb Please have a look - I think its an improvement. The issues sonarcube is complaining about are legacy, so nothing to fix in this PR. The overall code coverage increase, so should be ok from that standpoint too. TBD: The default for Constructing the options instance might be better to be done via a Builder pattern, so we'd be more flexible when extending it in the future without risking breaking changes. We could also default to some default instance rather than passing |
Ical.Net/Evaluation/MaxIncrementsExceededEvaluationException.cs
Outdated
Show resolved
Hide resolved
|
|
||
| public override IEnumerable<Period> Evaluate(CalDateTime referenceDate, CalDateTime? periodStart, CalDateTime? periodEnd, bool includeReferenceDateInResults) | ||
| public override IEnumerable<Period> Evaluate(CalDateTime referenceDate, CalDateTime? periodStart, CalDateTime? periodEnd, EvaluationOptions options) | ||
| { |
There was a problem hiding this comment.
No coverage, wondering when this method would get called
There was a problem hiding this comment.
Within Ical.Net not at all I assume, because the only place where it could be used is RecurringEvaluator, where we use Recurrable.RecurrenceDates.GetAllPeriodsByKind(). Remove?!
| /// If the specified number of increments is exceeded without finding a recurrence, an | ||
| /// exception of type <see cref="Ical.Net.Evaluation.MaxIncrementsExceededEvaluationException"/> will be thrown. | ||
| /// </remarks> | ||
| public int? MaxIncrementCount { get; set; } |
There was a problem hiding this comment.
As it's not a counter, would sth. like NoPatternMatchLimit be more intuitive?
Likewise sth. like NoPatternMatchLimitExceededException?
and TestMaxIncrementCount method
There was a problem hiding this comment.
Your new naming is the best
|
Looks great!
We already had exchanged some thoughts about indefinite, so let's use it as the default, but add an explicit hints in our xmldoc about the risk. Same with release notes, sure.
Balancing benefit and related effort the current way should be appropriate.
Pls. consider the comments above. |
…RRULEs according to the RFC. Ignore `includeRefDate...`
…xplicitly anyways where required.
…than failing silently.
minichma
force-pushed
the
work/minichma/feature/evaluation_config
branch
from
4047773 to
aa71c7f
Compare
|
|
@axunonb Thought a little about the naming, now choose |
Several improvements:
includeReferenceDateInResultsfromEvaluate()et al as it shouldn't be needed as the expected behavior is pretty well defined in the RFC. Only exception: What to happen if an RRULE doesn't match DTSTART, in which case we don't includeDTSTARTas we already didn't before.EvaluationOptionstype and pass it toCalendar.GetOccurrences()and relatedRecurrencePatternEvaluator.MaxIncrementCountwith a new configuration propertyEvaluationOptions.MaxIncrementCount.MaxIncrementsExceededEvaluationExceptionrather than stopping evaluation silently.Fixes #747