-
Notifications
You must be signed in to change notification settings - Fork 22
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
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. |
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. Just one change and we can merge.
src/Validation/Requires.cs
Outdated
} | ||
|
||
if (!hasElements) | ||
if (!values.GetEnumerator().MoveNext()) |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LOL. Oh! I thought you mean that it was explicitly wrapped in a try..finally. 🤦
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.
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. |
src/Validation/Requires.cs
Outdated
finally | ||
{ | ||
throw new ArgumentException(Format(Strings.Argument_EmptyArray, parameterName), parameterName); | ||
if (enumerator is IDisposable disposable) | ||
{ | ||
disposable.Dispose(); | ||
} | ||
} |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That gets the enumerator twice though. Wouldn't we want to avoid that?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shoot! You're right. It was.
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
This will close #47.