Added Enumerable.Append and Enumerable.Prepend#5947
Conversation
|
@stephentoub @terrajobst NOTE: "element" vs. "value" difference. It seems that "element" would be more consistent with other Enumerable methods. |
|
@weshaggard |
| } | ||
|
|
||
| [Fact] | ||
| public void SameResultsRepeatCallsIntQueryPrePend() |
There was a problem hiding this comment.
Nit: Prepend instead of PrePend
|
LGTM. Maybe consider adding the tests: [Fact]
public void ForcedToEnumeratorDoesntEnumeratePrepend()
{
var iterator = NumberRangeGuaranteedNotCollectionType(0, 3).Prepend(4);
// Don't insist on this behaviour, but check it's correct if it happens
var en = iterator as IEnumerator<int>;
Assert.False(en != null && en.MoveNext());
}
[Fact]
public void ForcedToEnumeratorDoesntEnumerateAppend()
{
var iterator = NumberRangeGuaranteedNotCollectionType(0, 3).Append(4);
// Don't insist on this behaviour, but check it's correct if it happens
var en = iterator as IEnumerator<int>;
Assert.False(en != null && en.MoveNext());
}While the regression this would catch is far from likely, considering the number of times custom iterators are used in |
|
@JonHanna Thanks! |
|
Why is |
|
|
||
| ie.Add(42); | ||
|
|
||
| Assert.Equal(prepended, ie.Prepend(4)); |
There was a problem hiding this comment.
Maybe this test should assert that GetEnumerator() wasn't called prior to the second element being iterated. I can't see someone finding a way that iterating it had any advantage (quite the opposite), but eagerly calling GetEnumerator() might possibly profile as a slight improvement (or simply an implementation short-cut with less typing!), would have worse potential problems than this, and would cover everything this covers too.
There was a problem hiding this comment.
The idea was primarily to catch attempts to "probe" the source for emptiness or something like that before yielding the prepended element. It could be a problem if the whole thing is used in TakeWhile and if fetching elements from the source is somehow observable.
Considering how simple this operator is, this kind of changes are unlikely to happen unnoticed. I just added a testcase since I thought about the scenario while making changes.
Overall, I think we should have enough tests here already.
There was a problem hiding this comment.
Yes, that's throwing out ideas, not an objection. I'm still confused by MatchSequencePattern not failing, though.
There was a problem hiding this comment.
I'm still confused by MatchSequencePattern not failing, though.
Probably because System.Linq.Expressions doesn't have a P2P reference to System.Linq and is building against the bits in the package rather than what's live in the repo.
cc: @weshaggard
There was a problem hiding this comment.
Ah. So it won't fail until after this is merged? I was pretty sure it failed before under similar circumstances.
There was a problem hiding this comment.
I will add the new operators to the exclusion list.
We can consider adding them to IQueryable, but not in this change. It is not 100% match anyways.
There was a problem hiding this comment.
No, I don't think they're a match, being akin to Repeat and Range in producing elements.
There was a problem hiding this comment.
Modulo the query operators that are not extension methods on IEnumerable<T>, as far as I know, the IQueryable<T> surface is complete, so we should likely bring it on par for these operators (which are extension methods) in a separate change as well.
There was a problem hiding this comment.
We should probably open an issue to discuss that, but I'm inclined to think that there's no good way to universally do that across providers except by casting back to IEnumerable<T> and calling into the methods here anyway, which this already provides for given the inheritance order.
|
Assuming we're ok with new surface area in corefx's System.Linq that's not in the full framework's (@weshaggard), LGTM. |
New public API in System.Linq Closes #2075
VSadov
force-pushed
the
prepend
branch
3 times, most recently
from
477fa7a to
9d7317b
Compare
|
test this please |
* Bumped up version of System.Linq * Added Append/Prepend to the MatchSequencePattern test in System.Linq.Expressions since the new operators are not avaialble on IQueryable. * added a test for combinations of Append, Prepend, Concat
|
test this please |
Added Enumerable.Append and Enumerable.Prepend
|
This PR actually failed but jenkins didn't catch it: @mmitche how are we evaluating build success/failure? |
|
@ericstj The return code from build.cmd (or whatever the build step was). 0 = pass, anything else = fail |
|
@mmitche that's not consistent with MSBuild's error reporting apparently. |
|
@ChadNedzlek I feel like you said that you saw where this particular error got downgraded from "big red text that scares developers" to "don't fail the build". Or did I misunderstand? |
|
I just recall seeing that in the PR that fixes the build break, there was a change from ContinueOnError="WarnAndContinue" to ContinueOnError="ErrorAndContinue". #6018, the changes to file "build.proj". We had previously explicitly been ignoring these failures |
|
@ChadNedzlek that was a different task for package restore https://github.com/dotnet/corefx/pull/6018/files#diff-3ad318a39efd856540e8cf813f20ec09R63. The PR also had my real fix for this issue by updating the package authoring. dotnet-bot@0487996 This one definitely has big red text that scares developers and shows up in the build summary. I suspect msbuild is still returning 0 because we don't return false from the task. We do this specifically because we don't want to halt the build. We can switch the task to return false, but then put ErrorAndContinue on the task to see if that results in an error and a non-zero return value without halting the build on the error. |
|
It sounds like we are hitting dotnet/buildtools#401 which I created and assigned to @ericstj a while ago when I hit something similar. |
Added Enumerable.Append and Enumerable.Prepend Commit migrated from dotnet/corefx@998d957
12 participants
New public API in System.Linq
Closes #2075