Skip to content
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

Return implementation #636

Merged
merged 37 commits into from
Nov 6, 2019
Merged

Return implementation #636

merged 37 commits into from
Nov 6, 2019

Conversation

MitchBodmer
Copy link
Contributor

@MitchBodmer MitchBodmer commented Nov 3, 2019

Here's a first go at the Return method. #615

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 submitting the PR! I've conducted a first review. It's a good start but what I found missing most were tests for all the members of the list returned by Return.

MoreLinq/Return.cs Outdated Show resolved Hide resolved
MoreLinq/Return.cs Outdated Show resolved Hide resolved
MoreLinq/Return.cs Outdated Show resolved Hide resolved
MoreLinq/Return.cs Outdated Show resolved Hide resolved
MoreLinq/Return.cs Outdated Show resolved Hide resolved
MoreLinq/Return.cs Outdated Show resolved Hide resolved
MoreLinq/Return.cs Outdated Show resolved Hide resolved
MoreLinq/Return.cs Outdated Show resolved Hide resolved
MoreLinq.Test/ReturnTest.cs Outdated Show resolved Hide resolved
MoreLinq.Test/ReturnTest.cs Outdated Show resolved Hide resolved
@atifaziz atifaziz changed the title Add return method. Return implementation Nov 4, 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.

Thanks for working through all changes from the previous review very promptly. This is a quick review for now with only a since issue that needs addressing.

MoreLinq/Return.cs Outdated Show resolved Hide resolved
TestIndexOfDoesNotThrowWhenTheItemContainedIsNull
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 adding all those tests! Looking much better and I think we are nearly there. 🏁

Also, could I ask you to add Return to the operators list in README.md and the project/package description?

MoreLinq/Return.cs Outdated Show resolved Hide resolved
MoreLinq/Return.cs Outdated Show resolved Hide resolved
MoreLinq/Return.cs Outdated Show resolved Hide resolved
MoreLinq.Test/ReturnTest.cs Outdated Show resolved Hide resolved
MoreLinq.Test/ReturnTest.cs Outdated Show resolved Hide resolved
MoreLinq.Test/ReturnTest.cs Outdated Show resolved Hide resolved
MoreLinq.Test/ReturnTest.cs Outdated Show resolved Hide resolved
MoreLinq.Test/ReturnTest.cs Show resolved Hide resolved
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.

This is beginning to really take up shape! Thanks for all those well-reading tests! There are just a few edits left now and then we're good to go with this!

I took the liberty to make a change myself with 236daef as it would have been too painful to describe in words. 😄 It associates the test cases with the test so this is how they appear named and organised:

image

MoreLinq/Return.cs Outdated Show resolved Hide resolved
MoreLinq/Return.cs Outdated Show resolved Hide resolved
MoreLinq.Test/ReturnTest.cs Outdated Show resolved Hide resolved
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 @MitchBodmer for working through the review and contributing this in the first place! Looking good to ship now! 🎉


PS I might make some minor formatting changes before merging.

@MitchBodmer
Copy link
Contributor Author

@atifaziz
Happy to do it. Thank you for being open to the feature and for the excellent code review.

@atifaziz atifaziz merged commit 6fc7657 into morelinq:master Nov 6, 2019
@Orace Orace mentioned this pull request Nov 11, 2019
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