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

Add min/max to same-element <T: Ord> EitherOrBoth #690

Closed
wants to merge 2 commits into from

Conversation

2ndDerivative
Copy link
Contributor

Had the idea of this low-cost quality of life implementation on EitherOrBoth.
Please give any feedback you have!
Cheers!

@phimuemue
Copy link
Member

Hi there and thank you for the idea.

I am unsure if min/max are special enough to justify the existence of auxiliary methods for them, so I thought about generalizing this and offering a function that the user can plug in min/Max/anything else into...

... just to find out this already exists: https://docs.rs/itertools/latest/itertools/enum.EitherOrBoth.html#method.reduce

So my personal take on this would be to go without the specific min/max unless we have evidence that users really need this.

@2ndDerivative
Copy link
Contributor Author

I saw that function too and the formulation of "Product" did not prepare me for what exactly it does. I thought it was about multiplication and was kind of irritated. I agree with the sentiment of not implementing this then, but would probably suggest improving the documentation for reduce()

@phimuemue phimuemue closed this Apr 14, 2023
bors bot added a commit that referenced this pull request Apr 14, 2023
693: Add example and improve docs for EitherOrBoth::reduce r=phimuemue a=2ndDerivative

Also changed "product" to avoid confusion with multiplication.

This was to address the confusion that led to PR #690 which can be closed in my opinion if this change is added instead. 
Hit me up for any feedback!

Co-authored-by: 2ndDerivative <niclas@klugmann.de>
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.

None yet

2 participants