Add Ical.Net.TimeZone namespace and related classes
#754
Conversation
Codecov ReportAttention: Patch coverage is @@ Coverage Diff @@
## main #754 +/- ##
====================================
Coverage 66% 67%
====================================
Files 106 109 +3
Lines 4464 4581 +117
Branches 1076 1098 +22
====================================
+ Hits 2962 3047 +85
- Misses 1057 1080 +23
- Partials 445 454 +9
... and 2 files with indirect coverage changes 🚀 New features to boost your workflow:
|
axunonb
force-pushed
the
axunonb/wip/pr/custom-timezone
branch
2 times, most recently
from
cd702af to
6286536
Compare
|
|
@minichma Please take a look at |
|
@axunonb I will be on vacation until Sunday. Will have a look when I'm back. |
minichma
left a comment
minichma
left a comment
There was a problem hiding this comment.
I had a very brief look at Occurrences_WithinUndefined_TzIntervals(). I assume, a TZ-related test for intervals where no TZ is defined only makes sense if we very clearly define what should happen in such case (which would most likely throwing an exception IMHO). But the more fundamental topic seems to be that VTimeZoneProvider is not fully implemented. I.e. it needs to consider RRULEs and RDATEs. Recommend to use TimeZoneEvaluator for that purpose.
I also recommend adding some tests that compare the time zone definitions from NodaTime to those generated by VTimeZoneProvider. The related TZ definitions could be taken from here. It would be best to compare all defined timezones, but then we'd have to use the exact same version of TZDB, which would require additional maintenance efforts in the future, so maybe just compare just some selected TZ that are unlikely to have their definitions like Europe/Paris, America/New_York or the like.
axunonb
force-pushed
the
axunonb/wip/pr/custom-timezone
branch
3 times, most recently
from
cfd68f5 to
7abddfb
Compare
axunonb
changed the title
VTimeZoneProvider for resolving timezones from VTIMEZONE componentsIcal.Net.TimeZone namespace and related classes
axunonb
force-pushed
the
axunonb/wip/pr/custom-timezone
branch
from
7abddfb to
1afeb36
Compare
|
@minichma This WIP is a rewrite of the initial PR for handling custom timezones, incorporating your recommendations. Unit tests using America_New_York.ics and America_Los_Angeles.ics from libical currently produce results consistent with NodaTime. Additional test cases are still needed. Do you have any feedback on this early preview? |
| var offsetTo = tzi.OffsetTo != null ? Offset.FromTimeSpan(tzi.OffsetTo.Offset) : Offset.Zero; | ||
| var savings = isDaylight ? (offsetTo - offsetFrom) : Offset.Zero; | ||
|
|
||
| foreach (var rrule in tzi.RecurrenceRules) |
There was a problem hiding this comment.
The whole evaluation logic is implemented in TimeZoneInfoEvaluator (which doesn't add any functionality over RecurringEvaluator and could therefore be removed), so I recommend reusing that.
There was a problem hiding this comment.
Oh yes, sure - sorry
axunonb
force-pushed
the
axunonb/wip/pr/custom-timezone
branch
2 times, most recently
from
2dfa32a to
ef244f4
Compare
minichma
left a comment
minichma
left a comment
There was a problem hiding this comment.
I didn't have the time for a proper review, especially in VTimeZoneExtensions, just a few thoughts for now.
| private void Add(DateTimeZone timeZone) | ||
| { | ||
| #if NET6_0_OR_GREATER | ||
| _timeZones.TryAdd(timeZone.Id, timeZone); |
There was a problem hiding this comment.
If adding is tolerant regarding duplicates, we should follow the .net conventions and name the method (and the calling ones) bool TryAdd(...). Otherwise we should throw.
| /// </summary> | ||
| /// <param name="timeZones"></param> | ||
| /// <param name="untilYear">The year to which timezone transitions will be calculated.</param> | ||
| public void AddRangeFrom(ICollection<VTimeZone> timeZones, int untilYear) |
There was a problem hiding this comment.
Not sure we need the untilYear here. We could just enumerate as needed.
| /// Adds a <see cref="DateTimeZone"/> to the provider if it does not already exist. | ||
| /// </summary> | ||
| /// <param name="timeZone">The <see cref="DateTimeZone"/> to add.</param> | ||
| private void Add(DateTimeZone timeZone) |
There was a problem hiding this comment.
Most often its preferable to make classes like this immutable. I.e. pass all the required details in the constructor. This simplifies the interface and complexity of the class significantly. I wouldn't see a reason why mutability would be required.
| public DateTimeZone? GetZoneOrNull(string id) | ||
| { | ||
| // Check for fixed-offset timezones, as described for IDateTimeZoneProvider | ||
| if (id.Equals(DateTimeZone.Utc.Id, StringComparison.OrdinalIgnoreCase)) |
There was a problem hiding this comment.
Hm, why would we add special treatment for UTC? This class is dedicated to deal with time zones based on VTimeZone. Wouldn't artificially add UTC here.
| if (id.Equals(DateTimeZone.Utc.Id, StringComparison.OrdinalIgnoreCase)) | ||
| return DateTimeZone.Utc; | ||
|
|
||
| if (id.StartsWith(DateTimeZone.Utc.Id, StringComparison.OrdinalIgnoreCase)) |
There was a problem hiding this comment.
Same here, this has nothing to do with VTimeZone. If we think, additional functionality is needed, this should be added outside of this class, e.g. by providing some utility function that allows combining multiple providers, where one could be an instance of this class and the other could deal with UTC. But without having thought about that in detail, I don't think this is something this project should deal with.
|
|
||
| // Handle instants before the first zone interval | ||
| if (_zoneIntervals.Count == 0 || instant < _zoneIntervals[0].Start) | ||
| return CreateZoneInterval( |
There was a problem hiding this comment.
If we don't have any time zone information, we shouldn't guess here. It's completely unclear, what the reason is for no time zone info being present. It could easily be that the time zone info has intentionally been limited to a certain time range, in which case returning some fallback based on heuristics will most likely be wrong. Just return either null or throw.
|
|
||
| // If we get here, the instant is *after all defined intervals* | ||
| // Return a default interval (this case shouldn't happen with proper data) | ||
| return CreateZoneInterval( |
There was a problem hiding this comment.
Same as above - if we don't know the actual data, throw or return null.
| Id); | ||
|
|
||
| // Binary search for the matching interval | ||
| var low = 0; |
There was a problem hiding this comment.
Isn't this already implemented in some NodaTime type we could reuse?
|
|
||
| var tzEvaluator = new TimeZoneInfoEvaluator(tzi); | ||
| var start = tzi.Start!; // Start is not null due to the filter above | ||
| var end = new CalDateTime(untilYear, 12, 13, 23, 59, 59); |
There was a problem hiding this comment.
Just noted that we have some inconsistency regarding periodEnd being inclusive or exclusive. I.e. in
periodEnd is handled exclusive (which I'd consider correct), while in
it is considered inclusive (which should be fixed).
(see #740)
There was a problem hiding this comment.
Hm, if we rewrite the existing condition
if (candidate < periodStart || candidate >= periodEnd)
{
if (periodStart != periodEnd || candidate > periodEnd)
{
continue;
}
}isn't periodEnd exclusive then, except for periodStart == periodEnd? (same as in RecurrenceUtil)?
There was a problem hiding this comment.
If I read it correctly, in this case periodEnd would still be treated inclusive and periodStart would be mostly ignored.
However, I feel the whole start/end evaluation is quite broken. If I'm right, RecurringEvaluator doesn't filter RDATEs at all, that's only done by RecurrenceUtil. So if you use the Evaluator directly as done in this PR, this might cause inconsistencies.
I'd recommend fixing this start/end topic first. Question is how to design the behavior. Actually its usually always a good idea and I'd heavily recommend to have the upper bound exclusive to avoid having to deal with rounding issues, epsilons, 23:59:59, .AddSecond(-1), etc. On the other hand the extra handling of start==end is also less than ideal. This special handling increases complexity quite significantly, as the caller always needs to consider this special case. But having periodEnd being exclusive AND avoiding the start==end special case makes it harder for the caller to ask for recurrences at a certain instant. An elegant solution would be to remove the periodEnd param altogether. Now that Evaluate and GetOccurrences return a generator IEnumerable, the callers can easily apply something like TakeWhile(p => p.StartDate <= endDate) themselves. Would also make quite some code significantly simpler. This would also come very handy, should we consider extending the evaluation functionality by something like evaluating backwards in the future. What do you think?
There was a problem hiding this comment.
Just a very quick (untested, unreviewed) sketch: https://github.com/minichma/ical.net/tree/work/minichma/wip/remove_period_end - nice how the complex limiting expressions go away :-)
There was a problem hiding this comment.
the extra handling of start==end is also less than ideal
Less than ideal is a cautious description for something we counter in several places with different, complex code
consider extending the evaluation functionality by something like evaluating backwards in the future.
Wow, perfect! And periodEnd or untilYear just fly into the past. Hope we get this implemented.
| public Offset Savings { get; } = savings; | ||
| } | ||
|
|
||
| private static bool TryCreateIntervals(VTimeZone vTimeZone, int untilYear, out string tzId, |
There was a problem hiding this comment.
Try to avoid the need for untilYear altogether? Alternatively just combine the individual IEnumerables to a single one using .OrderedMerge() and store the corresponding IEnumerator. When more data is needed, just continue the enumeration?
axunonb
force-pushed
the
axunonb/wip/pr/custom-timezone
branch
4 times, most recently
from
b9a2457 to
8592661
Compare
axunonb
force-pushed
the
axunonb/wip/pr/custom-timezone
branch
from
8592661 to
877ce63
Compare
|
Introduced new namespace
Ical.Net.TimeZonecontaining:TimeZoneResolver(just moved)VTimeZoneExtensions: Allow to convertVTimeZones toDateZimeZone(explicitly to internalCalDateTimeZone) keeping the calendar component leanCalDateTimeZoneinheriting formDateZimeZoneto handle timezones created fromVTimeZonesCalTimeZoneProviderto be used withTimeZoneResolverfor timezones included in VTIMEZONE iCalendar components