Skip to content

Conversation

@minichma
Copy link
Collaborator

@minichma minichma commented May 1, 2025

Remove the periodEnd param from Calendar.GetOccurrences(), IEvaluator.Evaluate() et al, which was used for specifying an upper bound when returning occurrences. Since #665 the returned sequence is a generator and only generates the results as it is iterated. Hence an upper bound can easily be implemented using Enumerable.TakeWhile() or the like.

The exact behavior of the param was not exactly easy to understand. It was often unclear, whether it is considered inclusive or exclusive (compare #740), there was special handling for the case periodStart==periodEnd, etc., which is removed with this PR and therefore significantly simplifies the interface and reduces complexity.

@minichma minichma force-pushed the work/minichma/feature/remove_period_end branch from 09c0e8a to de8ba14 Compare May 1, 2025 20:44
@codecov
Copy link

codecov bot commented May 1, 2025

Codecov Report

Attention: Patch coverage is 89.13043% with 5 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
Ical.Net/CalendarComponents/Alarm.cs 0% 2 Missing ⚠️
Ical.Net/CalendarCollection.cs 50% 1 Missing ⚠️
Ical.Net/CalendarComponents/RecurringComponent.cs 50% 1 Missing ⚠️
Ical.Net/VTimeZoneInfo.cs 0% 1 Missing ⚠️

❌ Your project status has failed because the head coverage (66%) is below the target coverage (80%). You can increase the head coverage or adjust the target coverage.

Impacted file tree graph

@@         Coverage Diff         @@
##           main   #781   +/-   ##
===================================
  Coverage    66%    66%           
===================================
  Files       105    105           
  Lines      4438   4431    -7     
  Branches   1115   1113    -2     
===================================
- Hits       2912   2910    -2     
+ Misses     1062   1058    -4     
+ Partials    464    463    -1     
Files with missing lines Coverage Δ
Ical.Net/Calendar.cs 72% <100%> (ø)
Ical.Net/CalendarComponents/FreeBusy.cs 48% <100%> (+1%) ⬆️
Ical.Net/Evaluation/Evaluator.cs 100% <ø> (ø)
Ical.Net/Evaluation/EventEvaluator.cs 88% <100%> (ø)
Ical.Net/Evaluation/RecurrencePatternEvaluator.cs 91% <100%> (+<1%) ⬆️
Ical.Net/Evaluation/RecurrenceUtil.cs 100% <100%> (+6%) ⬆️
Ical.Net/Evaluation/RecurringEvaluator.cs 93% <100%> (+1%) ⬆️
Ical.Net/Evaluation/TimeZoneInfoEvaluator.cs 33% <ø> (+13%) ⬆️
Ical.Net/Evaluation/TodoEvaluator.cs 56% <100%> (ø)
Ical.Net/CalendarCollection.cs 49% <50%> (ø)
... and 3 more
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@minichma minichma force-pushed the work/minichma/feature/remove_period_end branch from de8ba14 to 774f1b8 Compare May 1, 2025 20:55
@minichma minichma marked this pull request as ready for review May 1, 2025 21:07
@minichma minichma requested a review from axunonb May 1, 2025 21:07
p.StartTime.GreaterThanOrEqual(periodStart)
|| endTime.GreaterThan(periodStart)
select p;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

much more simple

axunonb
axunonb previously approved these changes May 6, 2025
Copy link
Collaborator

@axunonb axunonb left a comment

Choose a reason for hiding this comment

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

Significantly improves usability and removes ambiguities - thanks a lot!

@axunonb
Copy link
Collaborator

axunonb commented May 6, 2025

I'll publish a new pre-release after this PR is merged, okay?

@minichma minichma force-pushed the work/minichma/feature/remove_period_end branch from 774f1b8 to f141c0f Compare May 6, 2025 09:58
@minichma
Copy link
Collaborator Author

minichma commented May 6, 2025

I'll publish a new pre-release after this PR is merged, okay?

Sounds great. Whatever you think.

axunonb
axunonb previously approved these changes May 6, 2025
@axunonb
Copy link
Collaborator

axunonb commented May 6, 2025

Maybe change the parameter name as suggested by Sonar

…enceDate` to be consistent with the interface, as suggested by sonar
@sonarqubecloud
Copy link

sonarqubecloud bot commented May 6, 2025

Copy link
Collaborator

@axunonb axunonb left a comment

Choose a reason for hiding this comment

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

Excellent!

@axunonb axunonb merged commit d306ab5 into main May 6, 2025
8 of 9 checks passed
@axunonb axunonb deleted the work/minichma/feature/remove_period_end branch May 6, 2025 14:10
@minichma
Copy link
Collaborator Author

I'll publish a new pre-release after this PR is merged, okay?

@axunonb Do you think it would be a good time for the next pre-release? Would appreciate having a version with the culture bug fixed.

@axunonb
Copy link
Collaborator

axunonb commented May 21, 2025

Do you think it would be a good time for the next pre-release? Would appreciate having a version with the culture bug fixed.

I'll try to make it over the next 24 hours.
2025-05-21: ✅

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