Implement CollectionExtensions.TakeWhileBefore extensions#796
Conversation
…Before` Remove class `TestHelpers`
Codecov ReportAll modified and coverable lines are covered by tests ✅ ❌ 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 #796 +/- ##
===================================
Coverage 67% 67%
===================================
Files 105 106 +1
Lines 4419 4421 +2
Branches 1060 1060
===================================
+ Hits 2940 2942 +2
Misses 1045 1045
Partials 434 434
🚀 New features to boost your workflow:
|
axunonb
changed the title
CollectionExtensions.TakeBefore extensionsCollectionExtensions.TakeBefore extensions
| /// The elements of the sequence of occurrences to only include those that start before the specified period end. | ||
| /// </returns> | ||
| public static IEnumerable<Occurrence> TakeBefore(this IEnumerable<Occurrence> sequence, CalDateTime? periodEnd) | ||
| => (periodEnd == null) ? sequence : sequence.TakeWhile(p => p.Period.StartTime < periodEnd); |
There was a problem hiding this comment.
I think periodEnd shouldn't be nullable. In the test space I made it nullable to act as a c&p replacement for the previous periodEnd param and its used just internally (actually this was part of the reason not making it public). But in this case I feel its not quite natural allowing to pass a null value, making it a noop. I therefore suggest making it non-null. In the test projects we could keep an internal extension method that still allows passing null for simplicity.
There was a problem hiding this comment.
Actually this was my first thought as well.: null would be a noop. Just like "abc".ToString() returning this.
However, users could then use toDate e.g. in a loop, without caring for nullability. See our own tests below.
If toDate was not nullable, we could (would) write the unit test RecurrenceTests.EventOccurrenceTest(...) like
var occurrences = toDate != null
? evt.GetOccurrences(fromDate).TakeBefore(toDate).ToList()
: evt.GetOccurrences(fromDate).ToList();without the need for an extension method just for this test.
What's better?
There was a problem hiding this comment.
I'd prefer the not nullable, just like .TakeWhile or .Skip don't accept null. For our own use we can add the nullable version, like everyone can easily do. Outside of test cases I wouldn't expect it to be a common case that invocations need the nullable version. Either they want to limit or they don't I'd expect.
| /// <returns> | ||
| /// The elements of the sequence of periods to only include those that start before the specified period end. | ||
| /// </returns> | ||
| public static IEnumerable<Period> TakeBefore(this IEnumerable<Period> sequence, CalDateTime? periodEnd) |
There was a problem hiding this comment.
Regarding the naming: TakeBefore could be interpreted as filter (aka 'take all items from the sequence where the value is before x). In this interpretation it wouldn't reflect the fact that 'Before' refers to the items in the sequence, which means _take those that are returned before hitting or exceeding the threshold). An alternative could be TakeWhileBefore, which more clearly suggests 'take all items from the sequence while the their date value is before x', but the name is a bit clumsy. Not sure.
There was a problem hiding this comment.
Yes, really makes sense to think about the best term. Coming back to one of the initial ideas using like .GetOccurrences(...).WhileBefore(...) sounded quite natural, and it was short... Better?
There was a problem hiding this comment.
Sounds better to me. It's not quite in line with linq naming though. Not sure it has to be in this case. On the other hand, if it is named like TakeWhileXxx it could be easier to find, because it pops up in auto completion when people are looking for linq operators. Anyone, any of the discussed names is good.
There was a problem hiding this comment.
"easier to find" is convincing for .TakeWhileBefore(...), tnx
|
axunonb
changed the title
CollectionExtensions.TakeBefore extensionsCollectionExtensions.WhileTakeBefore extensions
axunonb
changed the title
CollectionExtensions.WhileTakeBefore extensionsCollectionExtensions.TakeWhileBefore extensions
|
@minichma BTW quite nice how code quality has improved since October 2024: From 274 issues to 105 now. |
Very nice to see. It seems lots of the improvements coming from NRT - great work! |
2 participants
These are convenience methods for
Enumerable.TakeWhile{TSource}(IEnumerable{TSource}, Func{TSource, bool}).Resolves #787