Skip to content

Conversation

@minichma
Copy link
Collaborator

GetOccurrences() used to ignore the TZ of periodStart, which is fixed by this PR.

fixes #840

@codecov
Copy link

codecov bot commented Jul 15, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Impacted file tree graph

@@           Coverage Diff           @@
##            main    #842     +/-   ##
=======================================
+ Coverage   67.7%   67.7%   +0.1%     
=======================================
  Files        106     106             
  Lines       4226    4224      -2     
  Branches     952     951      -1     
=======================================
+ Hits        2859    2860      +1     
  Misses      1038    1038             
+ Partials     329     326      -3     
Files with missing lines Coverage Δ
Ical.Net/Evaluation/RecurrencePatternEvaluator.cs 90.8% <ø> (ø)
Ical.Net/Evaluation/RecurrenceUtil.cs 100.0% <ø> (ø)

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@minichma
Copy link
Collaborator Author

minichma commented Jul 15, 2025

@ical-org/maintainers TBD

  • Although the behavior changes, I wouldn't consider this a breaking change but a bugfix.
  • Please check the test cases, especially those, where DTSTART is date-only but periodStart isn't and let me know whether you have any objections.

Note: The code is based on #836, so that should be merged first. Only the last two commits are specific to this PR, the rest comes from the other one.

axunonb
axunonb previously approved these changes Jul 15, 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.

Together with the new unit tests we should have all variants well covered, thanks!

This comment should be removed

// In the first step, we work with DateTime values, so we need to convert the CalDateTime to DateTime

@axunonb
Copy link
Collaborator

axunonb commented Jul 15, 2025

Although the behavior changes, I wouldn't consider this a breaking change but a bugfix.

Agree

See also: #840 (comment)

var cal = new Calendar();
cal.Events.Add(evt);

var firstFewOcccurrences = cal.GetOccurrences().Take(3).ToList();
Copy link
Collaborator

Choose a reason for hiding this comment

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

firstFewOccurrences

@minichma minichma dismissed axunonb’s stale review July 15, 2025 13:20

The merge-base changed after approval.

@minichma minichma force-pushed the work/minichma/bugfix/periodstart_tz branch from 2101064 to d20cf38 Compare July 15, 2025 13:20
@minichma minichma force-pushed the work/minichma/bugfix/periodstart_tz branch from 90e536a to 5ef193f Compare July 15, 2025 13:29
@sonarqubecloud
Copy link

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.

LGTM

@axunonb
Copy link
Collaborator

axunonb commented Jul 15, 2025

Ready for release 5.1.0 now?

@minichma minichma merged commit e9e9067 into main Jul 15, 2025
9 checks passed
@minichma minichma deleted the work/minichma/bugfix/periodstart_tz branch July 15, 2025 19:07
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.

GetOccurrences incorrectly changes start time zone

3 participants