Replaces the single-iteration foreach loops#48
Conversation
…numerator.MoveNext().
Codecov Report
@@ Coverage Diff @@
## master #48 +/- ##
==========================================
+ Coverage 88.88% 90.90% +2.02%
==========================================
Files 9 9
Lines 279 275 -4
==========================================
+ Hits 248 250 +2
+ Misses 31 25 -6
Continue to review full report at Codecov.
|
|
Not sure how you want to handle the codecov issue. As far as I can tell, it's saying that the coverage went down because there were lines of code removed. When I look at the diff, it looks like the new lines are being fully covered by tests. |
AArnott
left a comment
AArnott
left a comment
There was a problem hiding this comment.
Thanks. Just one change and we can merge.
| } | ||
|
|
||
| if (!hasElements) | ||
| if (!values.GetEnumerator().MoveNext()) |
There was a problem hiding this comment.
The old code would try casting the IEnumerator to IDisposable and dispose it if it did. It would do this in a try/finally to ensure it happened. We lost that in this method. Can you add it back?
There was a problem hiding this comment.
I can make this change, but I double-checked the code and didn't see anywhere that had it wrapped.
There was a problem hiding this comment.
I'm not sure what you mean. The try finally behavior I'm referring to comes automatically from the compiler by virtue of the foreach loop.
There was a problem hiding this comment.
LOL. Oh! I thought you mean that it was explicitly wrapped in a try..finally. 🤦
There was a problem hiding this comment.
I didn't think to wrap it initially because IEnumerator doesn't implement IDisposable, although IEnumerator<T> does.
…it also implements IDisposable.
|
Ok. Definitely not sure what you want to do about the codecov/patch failure here. It looks like it's complaining because the new call to dispose of the enumerator isn't being covered by a unit test. I can try and fabricate a unit test that will hit if you want me to though. |
|
I didn't mean to push that last commit (87e25c5) into this PR. :( Should I revert it? |
I wouldn't push a revert commit, but rather force-push the rollback so it's as if it never happened. |
| finally | ||
| { | ||
| throw new ArgumentException(Format(Strings.Argument_EmptyArray, parameterName), parameterName); | ||
| if (enumerator is IDisposable disposable) | ||
| { | ||
| disposable.Dispose(); | ||
| } | ||
| } |
There was a problem hiding this comment.
tip: a more succinct syntax for try/finally based disposal in this case can be achieved like this:
IEnumerator enumerator = values.GetEnumerator();
using (enumerator as IDisposable)
{
if (!values.GetEnumerator().MoveNext())
{
throw new ArgumentException(Format(Strings.Argument_EmptyArray, parameterName), parameterName);
}
}There was a problem hiding this comment.
That gets the enumerator twice though. Wouldn't we want to avoid that?
There was a problem hiding this comment.
Whoops. Well, that bug was already in your code. :) I just copied it in. Yes, let's avoid calling GetEnumerator() twice by actually using your enumerator local.
There was a problem hiding this comment.
Shoot! You're right. It was.
3 participants
This will close #47.