Enable NRT for ical.net (top) namespace
#771
Conversation
Codecov ReportAttention: Patch coverage is ❌ Your patch status has failed because the patch coverage (44%) is below the target coverage (80%). You can increase the patch coverage or adjust the target coverage. @@ Coverage Diff @@
## main #771 +/- ##
===================================
Coverage 67% 67%
===================================
Files 104 104
Lines 4490 4483 -7
Branches 1111 1103 -8
===================================
- Hits 2998 2994 -4
Misses 1064 1064
+ Partials 428 425 -3
🚀 New features to boost your workflow:
|
axunonb
force-pushed
the
wip/axunonb/pr/nrt-ical.net-namespace
branch
from
9058219 to
c19db97
Compare
axunonb
force-pushed
the
wip/axunonb/pr/nrt-ical.net-namespace
branch
from
c19db97 to
2d83ef3
Compare
minichma
left a comment
minichma
left a comment
There was a problem hiding this comment.
Very nice, a significant improvement!
A comment on making reviews somewhat more efficient: It would be helpful to keep changes apart in separate commits that can be reviewed independently. E.g. pure formatting changes (line breaks, spacing, ...), especially when applied by formatting tools, usually don't require the level of attention that functional changes do, so it would be helpful to have them in a separate commit that can be skimmed over very quickly. Functional changes, that are independent of each other could also be committed separately. Example:
- Commit: Pure formatting changes
- Commit: Unrelated functional change 1
- Commit: Unrelated functional change 2
- Commit: Preparatory functional change 1 required for implementing NRT
- Commit: Preparatory functional change 2 required for implementing NRT
- Commit: NRT
- Commit: Documentation changes
- ...
Not a big deal, just because time is very limited on all ends ;-)
Ical.Net/Calendar.cs
Outdated
| var recurrenceIdsAndUids = this.Children.OfType<IRecurrable>() | ||
| .Where(r => r.RecurrenceId != null) | ||
| .Select(r => new { (r as IUniqueComponent)?.Uid, Dt = r.RecurrenceId.Value }) | ||
| .Select(r => new { ((IUniqueComponent) r).Uid, Dt = r.RecurrenceId!.Value }) |
There was a problem hiding this comment.
What if r is not an IUniqueComponent? Previously we'd have returned null, now it would fail with an invalid cast.
Ical.Net/Calendar.cs
Outdated
| return (T) cal; | ||
| } | ||
| return default(T); | ||
| throw new ArgumentException($"Creating {typeof(T)} failed."); |
There was a problem hiding this comment.
Very nice! Not sure what Type.ToString() produces, but maybe specify something like typeof(T).FullName?
There was a problem hiding this comment.
$"{typeof(T)}" will indeed render the fully qualified name of the type T as a string
Ical.Net/CalendarCollection.cs
Outdated
| return this.Aggregate<Calendar, FreeBusy?>(null, (current, iCal) => | ||
| { | ||
| var freeBusy = iCal.GetFreeBusy(freeBusyRequest); | ||
| return current is null ? freeBusy : CombineFreeBusy(current, freeBusy!); |
There was a problem hiding this comment.
Hm, if freeBusy could be null here, we shouldn't apply !. Not sure, what the right behavior is but either GetFreeBusy() should return not-null or we should deal with nulls here.
Ical.Net/CalendarCollection.cs
Outdated
| return this.Aggregate<Calendar, FreeBusy?>(null, (current, iCal) => | ||
| { | ||
| var freeBusy = iCal.GetFreeBusy(organizer, contacts, fromInclusive, toExclusive); | ||
| return current is null ? freeBusy : CombineFreeBusy(current, freeBusy!); |
There was a problem hiding this comment.
freeBusy! - same as above
|
minichma
left a comment
minichma
left a comment
There was a problem hiding this comment.
Should the remaining/new null reference warnings be considered?
| var recurrenceIdsAndUids = this.Children.OfType<IRecurrable>() | ||
| .Where(r => r.RecurrenceId != null) | ||
| .Select(r => new { ((IUniqueComponent) r).Uid, Dt = r.RecurrenceId!.Value }) | ||
| .Select(r => new { (r as IUniqueComponent).Uid, Dt = r.RecurrenceId!.Value }) |
There was a problem hiding this comment.
This could throw with a npe now.
Yes, they should or rather: must. |
|
Thanks for your time and feedback. |
Enable NRT for
ical.net(top) namespace