-
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
Fix Memoize
Implementation
#898
Conversation
enumerator.MoveNextCalled += (_, moved) => | ||
{ | ||
Assert.That(ended, Is.False, "LINQ operators should not continue iterating a sequence that has terminated."); |
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.
Per the specification, it is not a failure to call .MoveNext()
after receiving a false
response. The enumerator is simply expected to continue to return false
for each following call.
@@ -69,14 +69,11 @@ public IEnumerator<T> GetEnumerator() | |||
_disposed = false; | |||
enumerator.Disposed += delegate | |||
{ | |||
Assert.That(_disposed, Is.False, "LINQ operators should not dispose a sequence more than once."); |
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.
Per the specification, it is not a failure to call .Dispose()
multiple times. The .Dispose()
method is expected to be idempotent, such that it is callable multiple times without throwing an exception.
Codecov Report
@@ Coverage Diff @@
## master #898 +/- ##
=======================================
Coverage 92.38% 92.38%
=======================================
Files 110 110
Lines 3441 3441
Branches 1020 1020
=======================================
Hits 3179 3179
Misses 200 200
Partials 62 62 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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.
There was no discussion in #889 and conclusion that there's any fixing needed, so I am going to hold off reviewing this for now until I have had time to think about the concerns you've raised.
Upon further review, this is the incorrect path to address concerns anyway. |
This PR does three things: 1) Correct issues in
Memoize
implementation; 2) Update tests forMemoize
andCartesian
to reflect updated expectations; 3) UpdateTestingSequence
with better expectations for how a testing sequence should be used.Fixes #889