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 max_by #379

Closed
wants to merge 9 commits into from
Closed

Add max_by #379

wants to merge 9 commits into from

Conversation

felipesere
Copy link
Contributor

Theoretically its was possibly to create an equivalent of max_by by using min_by.
That would have been awkward to read for users though.

I first tried to Box the comparison function and simply passing it down from max_by to min_by with reverse() applied. That got too tedious and meant users were exposed to the Box.

This variation adds a new field to the strut - now renamed MinMaxBy - that tells us whether we are maximising or minimising.

@felipesere
Copy link
Contributor Author

#129

@yoshuawuyts yoshuawuyts added the enhancement New feature or request label Oct 28, 2019
@yoshuawuyts
Copy link
Contributor

Hey @felipesere! -- thanks so much for this PR!

I noticed #384 partially overlaps with this PR. I would've left some notes on this PR, but basically #384 was exactly what I was hoping for these methods, so I've gone ahead and merged that instead.

I still want to take a moment to thank you for the work you've done here! -- I've seen you've put in a few patches, and they're all really good! I'm going to go ahead and close this PR in favor of #384, but please don't take it as a discouragement from continuing to contribute. I'm really stoked you've been helping out, and would love to continue to have you here!

Thanks so much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants