-
Notifications
You must be signed in to change notification settings - Fork 416
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce SkipLastWhile
extension
#1085
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1085 +/- ##
==========================================
+ Coverage 93.22% 93.27% +0.04%
==========================================
Files 112 113 +1
Lines 3410 3449 +39
Branches 962 971 +9
==========================================
+ Hits 3179 3217 +38
- Misses 215 216 +1
Partials 16 16
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a really good quality PR. Thanks for picking up #1085 and taking the trouble to conform to the project's conventions.
I've left a few comments & suggestion from my initial review.
Added missing period and added tags around "null" in docstrings Co-authored-by: Atif Aziz <code@raboof.com>
…sequence to an array
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for sharing your findings.
I have done a second review pass and submitted some suggestions and comments.
I reckon we need one final pass and then we can get this in! 🏁
PS Run build.cmd
or build.sh
locally to update the generated extensions since the CI checks/runs are failing due to stale code.
using NUnit.Framework; | ||
|
||
[TestFixture] | ||
public class SkipLastWhileTest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that SkipLastWhile
can be optimised for sequences, read-only lists and lists, you will want to test that the behaviour is identical irrespective of the source type. Have a look at some of the other tests to see how this is done, where we use TestExtensions.ToSourceKind
to help with this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Due to BreakingCollection
and BreakingReadOnlyCollection
not being list-like, the test cases using them fail since IterateSequence
uses a foreach loop, which will get the enumerator. Should I be testing for these cases? Considering that the IterateSequence
method does get covered when using the SourceKind.Sequence
test case. Also, do we want to test for different collection types on all of the applicable tests?
Adds
SkipLastWhile
extension method and associated testsCloses #1036