Skip to content

Conversation

@Orace
Copy link
Contributor

@Orace Orace commented Nov 8, 2019

Fix a part of #649
Fork of #660

SkipUntil called MoveNext after sequence end

Orace added 2 commits November 8, 2019 20:07
Avoid calls to MoveNext after enumerator end.
@Orace Orace changed the title Skip until SkipUntil : Add test, fix implementation Nov 8, 2019
Copy link
Member

@atifaziz atifaziz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this related to #649? It doesn't seem to have anything with SkipUntil forgetting to dispose the enumerator. Could you please explain what's the real issue being fixed here?

@Orace
Copy link
Contributor Author

Orace commented Nov 9, 2019

How is this related to #649?

It is not, sorry I copy paste to quickly. I updated the first comment..

It's a re-called MoveNext on sequence end issue.

@atifaziz
Copy link
Member

atifaziz commented Nov 9, 2019

It's a re-called MoveNext on sequence end issue.

So this is the fix for issue #664?

@Orace
Copy link
Contributor Author

Orace commented Nov 9, 2019

So this is the fix for issue #664?

Indeed, I forgot to mention it 😕

Copy link
Member

@atifaziz atifaziz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The TestSkipUntilOnKnownInput test doesn't exercise the bug that this PR proposes to fix. If I just revert the fix in 1abdb31 then all tests still pass. Can you please add a covering test first?

Thanks.

- empty sequence for input
- one-item sequence, predicate succeed
- one-item sequence, predicate don't succeed
- predicate succeed on first item
- predicate succeed on last item
- predicate never succeed
@Orace Orace requested a review from atifaziz November 12, 2019 08:48
Copy link
Member

@atifaziz atifaziz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are just minor cosmetic & polishing touches needed and then we're good to merge this.

Thanks!

@atifaziz atifaziz changed the title SkipUntil : Add test, fix implementation Fix SkipUntil to not call MoveNext past end of sequence Nov 12, 2019
* Update SkipUntil.cs: Formatting
* Update SkipUntilTest.cs: Test readability
Copy link
Contributor Author

@Orace Orace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will not compile without this.

@Orace
Copy link
Contributor Author

Orace commented Nov 12, 2019

A squash merge may reduce my shame

Copy link
Member

@atifaziz atifaziz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this fix! All looks good! :shipit:

@atifaziz atifaziz merged commit 13c8c83 into morelinq:master Nov 13, 2019
@Orace Orace deleted the SkipUntil branch November 20, 2019 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants