Let GetOccurrences() et al return an IEnumerable rather than HashSet.
#665
Conversation
| => this | ||
| .Select(iCal => iCal.GetOccurrences<T>(startTime, endTime)) | ||
| .ToArray() | ||
| .OrderedMergeMany(); | ||
|
|
||
| private FreeBusy CombineFreeBusy(FreeBusy main, FreeBusy current) |
There was a problem hiding this comment.
FreeBusy method are not referenced - do we still need them?
There was a problem hiding this comment.
I have no idea, but as they are not related to this PR, I suggest to deal with that in a different PR (see above).
|
Does this PR make #641 redundant, and could we remove |
Ical.Net/CalendarCollection.cs
Outdated
| } | ||
| return occurrences; | ||
| } | ||
| public IEnumerable<Occurrence> GetOccurrencesOfDay(DateTime dt) |
There was a problem hiding this comment.
Would you like to keep the unused (and thus uncovered overloads)?
There was a problem hiding this comment.
I generally didn't remove any methods, because I don't want to change the API in this PR outside of what is related to the immediate topic. If we want to change it, I think we should do that in a separate PR, so we keep concerns separated. But this one indeed is a special case. Actually I don't think, we should have a separate method just for getting the occurrences for a single day, because it can easily be implemented with the other variants and its not obvious, why we wouldn't also add versions for ByWeek, ByHour, etc. So I'd certainly vote for removing them, but also the other overloads (i.e. GetOccurrencesOfDay(IDateTime)).
In this case we have another challenge. Due to the params of GetOccurrences(DateTime? = null, DateTime? = null) et al being optional now, a call like GetOccurrences(new DateTime(...)) would now invoke the overload with startTime, endTime, rather than the overload for getting the occurrences for a single day (which has been renamed). This could break existing code without being obvious. It will be introduced in v5, so we could argue that's ok (and I actually would do so), but we should be aware of it. And add a note to a potential migration guide. What do you think?
There was a problem hiding this comment.
@axunonb I removed the versions of GetOccurrences(date) that were handling a single day altogether (see prev comment). Only two tests existed that tested any of them at all. I don't think, they will be missing. We should add some comment in a migration guide, should one be written, about the ambiguity due to the new nullable params in the other overloads.
|
@axunonb Thank you for the review, glad you like the change. Appreciate your comments. My previous few PRs have been based on branches in the ical-org repo, this one is based on my minichma repo. When based on the ical-org, a code coverage report is automatically generated, but obviously not if based on a different one. Could we change that, so we get the report for every PR, also when based on an external repo? |
Actually it doesn't, because |
Unfortunately we can't, as GitHub secrets can't be applied to external PRs, Just continue with branches inside ical-org. |
…ble`s, `OrderedMerge`, `OrderedMergeMany`, `OrderedDistinct`.
…etc. to return an ordered `IEnumerable` rather than a `HashSet` and modify the implementations to generate results on iteration rather than fully enumerating the full result set upfront.
minichma
force-pushed
the
work/minichma/feature/evaluate_enumerable
branch
from
946b25b to
a2ec4fa
Compare
|
Comments related to the new issues identified by Sonarcloud:
They aren't useless, because they cause an intended iteration. The reason is mentioned in the code comments.
TBD. I actually left the default params out intentionally. In the interface they are there for the convenience of users, but internally I think we should be more verbose and explicitly specify the params.
Not a topic of this PR. Some simplification has already been implemented in the mentioned method though. |
Yep, so we could mark the issues deliberately chosen as described in the comments with //NOSONAR (or a warning disable pragma, and leave the To-dos as issues to remember. |
…occurrences of a single day to `GetOccurrencesOfDay()` to avoid ambiguities with the other overloads.
minichma
force-pushed
the
work/minichma/feature/evaluate_enumerable
branch
from
9505ff2 to
2449741
Compare
|
Tweaked a little. Not too convinced this is helpful for the overall code quality. I'm not the biggest fan of cluttering code with 'disable' comments, so I undid a few changes. We might want to reconsider some of the rules over time. |
Absolutely, we're currently using the default "Sonar Way" rules. |
This PR modifies
Calendar.GetOccurrences()and related methods to return an orderedIEnumerablethat generates the actual occurrences as they are enumerated, rather than returning a fully populatedHashSet. When invokingGetOccurrences, the start- and endTime can now be omitted. The change brings several benefits:GetOccurrences(startTime: t).GetFirstOrDefault()Modifications implemented:
IEnumerablefromGetOccurrencesandIEvaluator.EvaluatestartTimeandendTimewhen callingGetOccurrencesGetOccurrencesthat returns the occurrences of a single day toGetOccurrencesOfDay, to avoid ambiguities with the other overloads, now that start/end time can be omitted.