Add LINQ APIs for Index and Range (#28776)#47950
Conversation
|
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
|
Tagging subscribers to this area: @eiriktsarpalis Issue DetailsAdd LINQ APIs for index and range:
Close #28776.
|
| select x; | ||
|
|
||
| Assert.Equal(q.ElementAtOrDefault(3), q.ElementAtOrDefault(3)); | ||
| var q = Repeat(_ => from x in new[] { 9999, 0, 888, -1, 66, -777, 1, 2, -12345 } |
There was a problem hiding this comment.
Curious what the motivation for this method is. It took me a while to grasp but it seems to be creating 3 identical IEnumerable instances? Why would that be needed?
There was a problem hiding this comment.
The original unit test creates a source, then calls ElementAtOrDefault twice on the same source, and asserts the result.
I am keeping the consistency.
There was a problem hiding this comment.
Sure, but for all intents and purposes the source enumerable in the test is immutable. We should not expect noticeable difference no matter how many times it has been enumerated. I would recommend keeping the tests as simple as possible (in this case and most other cases using the Repeat method, q can be enumerated as many times as needed. Assuming this were not the case, perhaps MemberData is preferable?
| var en1 = iterator1 as IEnumerator<int>; | ||
| Assert.False(en1 != null && en1.MoveNext()); | ||
|
|
||
| var iterator2 = NumberRangeGuaranteedNotCollectionType(0, 3).Take(2); |
There was a problem hiding this comment.
Why is this code duplicated?
| var en1 = iterator1 as IEnumerator<int>; | ||
| Assert.False(en1 != null && en1.MoveNext()); | ||
|
|
||
| var iterator2 = NumberRangeGuaranteedNotCollectionType(0, 3).ToList().Take(2); |
|
|
||
| var source = new DelegateIterator<int>( | ||
| moveNext: () => ++state <= sourceCount, | ||
| var source = Repeat(index => new DelegateIterator<int>( |
There was a problem hiding this comment.
This is the only test I could find where Repeat is really needed.
Nevertheless the test has too many moving parts and each section is applying different sets of assertions. I would recommend either breaking this up into separate unit tests or perhaps it might be possible to factor out some of the state management into a local method.
eiriktsarpalis
left a comment
eiriktsarpalis
left a comment
There was a problem hiding this comment.
Looks good to me, pending some feedback in the test code.
Dixin
force-pushed
the
linq-index-range
branch
from
f56645b to
da828a2
Compare
|
Did you intend to close this? Seemed almost ready to merge. |
|
@stephentoub No. Something went wrong when I manipulate my local git repo and push. Can you please reopen it? |
|
GitHub won't let me. I think the branch must have gotten messed up. Can you open a new PR? |
|
@stephentoub @eiriktsarpalis Please use PR #48559.
Yes. I did |
|
No worries. Thanks. |
ghost
locked as resolved and limited conversation to collaborators
3 participants
Add LINQ APIs for index and range:
Close #28776.