Enable NRT for `AlarmOccurrence`,`Attachment`, `Attendee`, `CalendarDataType`, `Duration`, `ICalendarDataType`, `ICalendarParameterCollectionContainer` #762

axunonb · 2025-04-07T11:57:07Z

Now < 100 files left for NRT :) - 30% done

codecov · 2025-04-07T11:58:43Z

Codecov Report

Attention: Patch coverage is 35.71429% with 18 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
Ical.Net/DataTypes/AlarmOccurrence.cs	0%	5 Missing ⚠️
Ical.Net/CalendarComponents/Alarm.cs	0%	4 Missing ⚠️
Ical.Net/DataTypes/Attachment.cs	43%	1 Missing and 3 partials ⚠️
Ical.Net/DataTypes/PeriodList.cs	33%	0 Missing and 2 partials ⚠️
Ical.Net/DataTypes/Attendee.cs	0%	1 Missing ⚠️
Ical.Net/DataTypes/CalendarDataType.cs	67%	1 Missing ⚠️
Ical.Net/DataTypes/Duration.cs	75%	1 Missing ⚠️

❌ Your patch status has failed because the patch coverage (36%) 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 (67%) is below the target coverage (80%). You can increase the head coverage or adjust the target coverage.

@@         Coverage Diff         @@
##           main   #762   +/-   ##
===================================
- Coverage    67%    67%   -0%     
===================================
  Files       104    104           
  Lines      4508   4498   -10     
  Branches   1103   1107    +4     
===================================
- Hits       3012   3001   -11     
- Misses     1065   1068    +3     
+ Partials    431    429    -2

Files with missing lines	Coverage Δ
Ical.Net/DataTypes/Organizer.cs	`44% <100%> (ø)`
Ical.Net/Evaluation/TodoEvaluator.cs	`62% <ø> (ø)`
Ical.Net/DataTypes/Attendee.cs	`70% <0%> (+2%)`	⬆️
Ical.Net/DataTypes/CalendarDataType.cs	`49% <67%> (+3%)`	⬆️
Ical.Net/DataTypes/Duration.cs	`71% <75%> (-6%)`	⬇️
Ical.Net/DataTypes/PeriodList.cs	`84% <33%> (-4%)`	⬇️
Ical.Net/CalendarComponents/Alarm.cs	`16% <0%> (-1%)`	⬇️
Ical.Net/DataTypes/Attachment.cs	`76% <43%> (-1%)`	⬇️
Ical.Net/DataTypes/AlarmOccurrence.cs	`0% <0%> (ø)`

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot

Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (2)

Ical.Net/Evaluation/TimeZoneInfoEvaluator.cs:27

The null-check for TimeZoneInfo was removed; consider restoring or adjusting this check to prevent potential NullReferenceExceptions if TimeZoneInfo remains unset.

if (TimeZoneInfo == null)

Ical.Net/DataTypes/PeriodList.cs:131

Using the null-conditional operator may silently fail to remove an item if Periods is null; ensure that Periods is initialized or handle the null case explicitly.

public void RemoveAt(int index) => Periods?.RemoveAt(index);

Copilot · 2025-04-07T12:04:03Z

Ical.Net/DataTypes/AlarmOccurrence.cs

    }

-    public int CompareTo(AlarmOccurrence other) => Period.CompareTo(other.Period);
+    public int CompareTo(AlarmOccurrence? other)


[nitpick] In the CompareTo method, returning 1 when other or its Period is null could result in inconsistent ordering; consider defining explicit ordering when both Period properties are null.

…ainer`

minichma

Very nice, just a few minor comments.

minichma · 2025-04-07T16:28:32Z

Ical.Net/DataTypes/Duration.cs

    /// </summary>
    /// <exception cref="System.FormatException">Thrown if the value is not a valid duration.</exception>
-    public static Duration Parse(string value) => (Duration)new DurationSerializer().Deserialize(new StringReader(value));
+    public static Duration? Parse(string value) =>


Unfortunately we are somewhat inconsistent here. According to the XML doc the method should throw if the passed string is not a valid duration. DurationSerializer throws in some cases and returns null in other. I'd generally suggest to throw and return a non-nullable Duration.

minichma · 2025-04-07T16:32:25Z

Ical.Net/DataTypes/PeriodList.cs

        foreach (var p in list)
        {
-            Add(p.Copy<Period>());
+            if (p != null)


By not copying null values the copy will not be equal to the original. Maybe to do something like Add(p?.Copy<Period>())?

minichma · 2025-04-07T16:33:51Z

Ical.Net/DataTypes/PeriodList.cs

        EnsureConsistentTimezoneAndPeriodKind(item);
        if (Periods.Contains(item)) return;
-        Periods.Insert(index, item);
+        Periods?.Insert(index, item);


This ? hast just been removed one commit earlier.

minichma · 2025-04-07T16:39:19Z

Ical.Net/DataTypes/Attendee.cs

                                             && Members.SequenceEqual(other.Members)
-                                             && DelegatedTo.SequenceEqual(other.DelegatedTo)
-                                             && DelegatedFrom.SequenceEqual(other.DelegatedFrom);
+                                             && (DelegatedTo?.SequenceEqual(other.DelegatedTo ?? Enumerable.Empty<string>()) ?? other.DelegatedTo == null)


This comparison is not commutative this way. I.e. if one is null and the other is empty, then the comparison is not symmetrical. Not sure though, whether an empty sequence should equal null. Usually they're considered different.

minichma · 2025-04-07T16:41:41Z

Ical.Net/DataTypes/Attachment.cs

        return Data == null
            ? firstPart
-            : firstPart && Data.SequenceEqual(other.Data);
+            : firstPart && other?.Data != null && Data.SequenceEqual(other.Data);


This comparison is not correct. If other?.Data == null, then this.Data is ignored, even if it contains data.

minichma · 2025-04-07T16:43:15Z

Ical.Net.Tests/RecurrenceTests.cs


-        var recurringPeriods1 = rpe1.Evaluate(new CalDateTime(start), start, end, default).ToList();
-        var recurringPeriods2 = rpe2.Evaluate(new CalDateTime(start), start, end, default).ToList();
+        var recurringPeriods1 = rpe1.Evaluate(new CalDateTime(start), start, end, null).ToList();


Yes, better. I started with default because the options should have been a value type at first.

minichma · 2025-04-07T16:47:16Z

Ical.Net/DataTypes/AlarmOccurrence.cs

-    public int CompareTo(AlarmOccurrence other) => Period.CompareTo(other.Period);
+    public int CompareTo(AlarmOccurrence? other)
+    {
+        if (other == null || other.Period == null)


What if other.Period == null and this.Period == null?

axunonb · 2025-04-07T17:56:52Z

Love your reviews, thanks!

minichma

Very nice!

minichma · 2025-04-07T18:48:01Z

Ical.Net/DataTypes/Duration.cs

-    public static Duration? Parse(string value) =>
-        (Duration?) new DurationSerializer().Deserialize(new StringReader(value));
+    public static Duration Parse(string value) =>
+        (Duration) new DurationSerializer().Deserialize(new StringReader(value))!; // throws if null


Actually it doesn't throw in all cases as of today (

ical.net/Ical.Net/Serialization/DataTypes/DurationSerializer.cs

Line 73 in 3c4e95b

return null;

) although it should.

sonarqubecloud · 2025-04-07T19:11:46Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

axunonb added 4 commits April 7, 2025 13:02

Enable NRT for AlarmOccurrence

7735978

Enable NRT for Attachment

c659a72

Enable NRT for Attendee

5592cc9

Enable NRT for CalendarDataType

d10d886

Enable NRT for Duration

d482732

axunonb force-pushed the axunonb/wip/pr/datatypes-nrt branch from d2d1822 to d482732 Compare April 7, 2025 12:00

axunonb marked this pull request as ready for review April 7, 2025 12:03

axunonb requested review from Copilot and minichma and removed request for Copilot April 7, 2025 12:03

Copilot AI reviewed Apr 7, 2025

View reviewed changes

Enable NRT for ICalendarDataType, `ICalendarParameterCollectionCont…

639b47f

…ainer`

axunonb changed the title ~~Enable NRT for AlarmOccurrence,Attachment, Attendee, CalendarDataType, Duration~~ Enable NRT for AlarmOccurrence,Attachment, Attendee, CalendarDataType, Duration, ICalendarDataType, ICalendarParameterCollectionContainer Apr 7, 2025

Enable NRT for IEncodableDataType

78f6218

axunonb force-pushed the axunonb/wip/pr/datatypes-nrt branch from ff53acc to 78f6218 Compare April 7, 2025 13:28

PeriodList.CopyFrom: Remove null check

80c8287

minichma reviewed Apr 7, 2025

View reviewed changes

Modifications from review

4e0333c

axunonb requested a review from minichma April 7, 2025 17:59

minichma previously approved these changes Apr 7, 2025

View reviewed changes

Merge branch 'main' into axunonb/wip/pr/datatypes-nrt

030f32f

axunonb dismissed minichma’s stale review via 030f32f April 7, 2025 19:11

axunonb merged commit e44021e into main Apr 7, 2025
5 of 7 checks passed

axunonb deleted the axunonb/wip/pr/datatypes-nrt branch April 7, 2025 19:12

This was referenced Jul 28, 2025

Bump the all-updates group with 4 updates mikkelkaas/HaandboldGrabber#13

Closed

Bump Ical.Net from 4.3.1 to 5.1.0 Atrejoe/Inky-Calendar-Server#243

Closed

dependabot bot mentioned this pull request Aug 4, 2025

Bump Ical.Net and 5 others Atrejoe/Inky-Calendar-Server#245

Closed

dependabot bot mentioned this pull request Aug 18, 2025

Bump the all-updates group with 4 updates mikkelkaas/HaandboldGrabber#14

Merged

dependabot bot mentioned this pull request Aug 25, 2025

Bump Ical.Net from 4.3.1 to 5.1.0 Atrejoe/Inky-Calendar-Server#246

Closed

dependabot bot mentioned this pull request Sep 1, 2025

Bump Ical.Net from 4.3.1 to 5.1.0 Atrejoe/Inky-Calendar-Server#253

Closed

dependabot bot mentioned this pull request Oct 13, 2025

Bump Ical.Net from 4.3.1 to 5.1.1 Atrejoe/Inky-Calendar-Server#257

Open

This was referenced Oct 23, 2025

Bump Ical.Net from 4.3.1 to 5.1.1 joinrpg/joinrpg-net#3675

Closed

Bump Ical.Net from 4.3.1 to 5.1.1 loresoft/InstructorIQ#379

Merged

Bump Ical.Net from 4.3.1 to 5.1.1 joinrpg/joinrpg-net#3707

Closed

Enable NRT for AlarmOccurrence,Attachment, Attendee, CalendarDataType, Duration, ICalendarDataType, ICalendarParameterCollectionContainer #762

Enable NRT for AlarmOccurrence,Attachment, Attendee, CalendarDataType, Duration, ICalendarDataType, ICalendarParameterCollectionContainer #762

Uh oh!

Conversation

axunonb commented Apr 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Apr 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 7, 2025

Choose a reason for hiding this comment

Uh oh!

minichma left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

axunonb commented Apr 7, 2025

Uh oh!

minichma left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Apr 7, 2025

Quality Gate passed

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Enable NRT for `AlarmOccurrence`,`Attachment`, `Attendee`, `CalendarDataType`, `Duration`, `ICalendarDataType`, `ICalendarParameterCollectionContainer` #762

Enable NRT for `AlarmOccurrence`,`Attachment`, `Attendee`, `CalendarDataType`, `Duration`, `ICalendarDataType`, `ICalendarParameterCollectionContainer` #762

axunonb commented Apr 7, 2025 •

edited

Loading

codecov bot commented Apr 7, 2025 •

edited

Loading