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

Make the time zone resolver plugable #732

Merged
merged 4 commits into from
Feb 16, 2025

Conversation

minichma
Copy link
Collaborator

This PR implements several improvements related to resolving time zones:

  • Fix test cases so they use well defined and known time zone IDs, so they don't fall back to the system's local TZ
  • Stop falling back to the local TZ if the specified TZ is unknown. Its more than likely that in most cases the fallback is not what is expected/desired by the user.
  • Make the time zone resolving functionality plugable

Copy link

codecov bot commented Feb 15, 2025

Codecov Report

Attention: Patch coverage is 68.29268% with 13 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
Ical.Net/DefaultTimeZoneResolver.cs 68% 3 Missing and 8 partials ⚠️
Ical.Net/CalendarComponents/VTimeZone.cs 33% 2 Missing ⚠️

❌ 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.
❌ 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   #732   +/-   ##
===================================
  Coverage    66%    66%           
===================================
  Files       103    105    +2     
  Lines      4644   4645    +1     
  Branches   1152   1154    +2     
===================================
+ Hits       3064   3066    +2     
+ Misses     1144   1143    -1     
  Partials    436    436           
Files with missing lines Coverage Δ
Ical.Net/TimeZoneResolvers.cs 100% <100%> (ø)
Ical.Net/Utility/DateUtil.cs 48% <100%> (-8%) ⬇️
Ical.Net/CalendarComponents/VTimeZone.cs 73% <33%> (ø)
Ical.Net/DefaultTimeZoneResolver.cs 68% <68%> (ø)

@minichma minichma marked this pull request as ready for review February 15, 2025 21:58
@minichma minichma requested a review from axunonb February 15, 2025 21:58
@minichma
Copy link
Collaborator Author

Relates to the discussion in #606 (reply in thread)

@@ -2291,7 +2291,7 @@ public void Yearly1()
public void Bug2912657()
{
var iCal = Calendar.Load(IcsFiles.Bug2912657);
var localTzid = iCal.TimeZones[0].TzId;
var localTzid = iCal.Events.First().Start.TzId;
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

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.

Great idea!

@minichma minichma merged commit 89500c0 into main Feb 16, 2025
6 of 8 checks passed
@minichma minichma deleted the work/minichma/feature/timezone_resolver branch February 16, 2025 08:40
@minichma
Copy link
Collaborator Author

Great idea!

Thanks and thanks for the review! Having global static parameters is always a somewhat poor design but in this case it seems to be the best short term solution without refactoring the whole evaluation code. At least for our project its a viable workaround. Could/should be improved in v6.

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.

2 participants