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(compat): Implement compat/pickBy #950

Merged
merged 7 commits into from
Mar 9, 2025
Merged

Conversation

gs18004
Copy link
Contributor

@gs18004 gs18004 commented Feb 2, 2025

I added the pickBy function to compat, which behaviors similar to Lodash's pickBy:
the second param of the function is optional.

Thanks to: #946

@gs18004 gs18004 requested a review from raon0211 as a code owner February 2, 2025 10:05
Copy link

vercel bot commented Feb 2, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
es-toolkit ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 9, 2025 8:37am

@gs18004 gs18004 changed the title feat(pickBy): Implement compat/pickBy feat(compat): Implement compat/pickBy Feb 2, 2025
* const result = pickBy(obj, shouldPick);
* // result will be { b: 'pick' }
*/
export function pickBy<T extends Record<string, any>>(

Choose a reason for hiding this comment

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

Hi!
If you could add small fix here it would be great.

Function doesn't warn you if you pass null as obj, and if you do so, it will crash :)

however, lodash pickBy in that case returns {}
you can look here:
https://github.com/lodash/lodash/blob/4.17.15/lodash.js#L13582

Please add same here, or some warning that obj can't be null.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your suggestion! I fixed this problem. Would you check it?

Choose a reason for hiding this comment

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

Great! Thank you very much)

Maybe add some tests for it? I'm not sure.

And wait for approval from someone from repo)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! I will add some tests :)

raon0211
raon0211 previously approved these changes Mar 9, 2025
Copy link
Collaborator

@raon0211 raon0211 left a comment

Choose a reason for hiding this comment

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

Thanks so much for your help! Moved some test cases from lodash.

@raon0211 raon0211 merged commit 4a656bb into toss:main Mar 9, 2025
1 of 7 checks passed
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