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

feat(collections): add minBy #1157

Merged
merged 1 commit into from
Aug 31, 2021
Merged

feat(collections): add minBy #1157

merged 1 commit into from
Aug 31, 2021

Conversation

majidsajadi
Copy link
Contributor

Implemented the minBy method in the collection module. ref: #1065

Types are declare based on this comment

Let me know if I should add any specific test cases.

Copy link
Contributor

@LionC LionC left a comment

Choose a reason for hiding this comment

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

All comments from my maxBy review apply here as well

@majidsajadi
Copy link
Contributor Author

@LionC changes from maxBy applied here too.

@LionC LionC mentioned this pull request Aug 27, 2021
44 tasks
@LionC
Copy link
Contributor

LionC commented Aug 27, 2021

Hey @majidsajadi . I am really sorry that I missed that before, but I think we should be consistent with the types we allow for orderings according to sortBy. Do you think you could add those?

I think the best way to do that is actually an overload instead of a giant generic thing (which I will change in sortBy myself). It should not be too hard to do, but hmu on Discord if you need help.

The same goes for the other max/min-By/Of PRs :-S

Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

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

@majidsajadi Thank you for your contribution! LGTM

@LionC Let's address typing issue in later PRs.

@kt3k kt3k merged commit 5bdd9f9 into denoland:main Aug 31, 2021
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.

3 participants