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.

Sign up for GitHub

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

Jump to bottom

feat(collections): Add pick and omit #4218

Merged
merged 2 commits into from
Feb 25, 2024
Merged

feat(collections): Add pick and omit #4218

merged 2 commits into from
Feb 25, 2024

Conversation

lambdalisue
Copy link
Contributor

This is like Lodash's pick and omit, but it's a bit more restrictive. Specifically, it only accepts an array of strings (string[]) rather than just a single string.

@lambdalisue lambdalisue requested a review from kt3k as a code owner January 17, 2024 13:36
@lambdalisue lambdalisue force-pushed the collections-pick-omit branch from 287bb50 to e51c1a6 Compare January 17, 2024 13:38
@timreichen
Copy link
Contributor

Isn't this very similar to filterKeys()?

@kt3k
Copy link
Member

kt3k commented Jan 18, 2024

Isn't this very similar to filterKeys()?

Look like a bit restricted versions of filterKeys with better typing when T and K are more specific than just Record<string, X> and string

I'm slightly in favor of including this

@kt3k kt3k added the feedback welcome We want community's feedback on this issue or PR label Jan 18, 2024
@lambdalisue
Copy link
Contributor Author

Ah, I didn't notice that there is filterKeys(). Thanks for mention. However, I still prefer pick and omit while TypeScript natively provides Pick and Omit type functions so those pair functions are more natural to me.

@lambdalisue lambdalisue force-pushed the collections-pick-omit branch from e51c1a6 to 04eb6c6 Compare January 18, 2024 08:59
petamoriken
petamoriken reviewed Feb 13, 2024
View reviewed changes
collections/omit.ts Outdated Show resolved Hide resolved
collections/pick.ts Outdated Show resolved Hide resolved
lambdalisue and others added 2 commits February 13, 2024 17:19
@lambdalisue lambdalisue force-pushed the collections-pick-omit branch from 86b8497 to ef8df5d Compare February 13, 2024 08:19
kt3k
kt3k approved these changes Feb 13, 2024
View reviewed changes
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.

Looks like we have more positive feedbacks to this.

LGTM

@Leokuma
Copy link

Leokuma commented Feb 13, 2024

I'm -1 for the following reasons:

  1. What Omit and Pick do is too similar to filterKeys. We'll end up with 3 functions that do the same thing but have completely different naming criteria and usage.
  2. While the names "Omit" and "Pick" align with TS, they disalign with JS' Array.filter. From that perspective, filterKeys sounds more consistent. Also, TS' Omit and Pick are used in the context of typing, not runtime. In the context of typing, the names "Pick" and "Omit" sound intuitive, but in runtime it's not so obvious what they do if you don't take TS into account, e.g. it's not clear that they deal with keys and not values.

Now that I'm thinking about it, I'm not even sure why we have filterKeys and filterValues and filterEntries. Couldn't we have only filterEntries, renamed to just filter? And its callback would take 2 args (key and value) instead of one array, just like Array.filter. Sorry if this has already been discussed. I think std/collections already has quite a lot of functions.

@lambdalisue
Copy link
Contributor Author

lambdalisue commented Feb 14, 2024 •
edited
Loading

@Leokuma

Thank you for providing your perspective. The statement below does not align with the discussed topic but reflects my personal view.

Both filterKeys and filter you've suggested offer a predicate function, leading to a non-deterministic return type. While Deno is undoubtedly a JavaScript runtime, it also functions as a TypeScript runtime, and I believe it should offer functions relevant to the TypeScript context. Consequently, I disagree with the notion that it can be replaced solely based on the existence of filterKeys-type functions.

@Leokuma
Copy link

Leokuma commented Feb 14, 2024

Sorry, I didn't notice the return type was different from filterKeys. In this case I'm in favor of this because it's quite cumbersome to type the return value of filterKeys in that use case. And it makes sense to align it with TS.

My question about filterKeys, filterEntries and filterValues still stands, but I can open another issue for that.

iuioiua
iuioiua approved these changes Feb 22, 2024
View reviewed changes
Copy link
Contributor

@iuioiua iuioiua left a comment

Choose a reason for hiding this comment

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

I like the idea! Cleanly done. LGTM.

@kt3k
Copy link
Member

kt3k commented Feb 25, 2024

Landing as there seem only supportive opinions from the community and the core team.

@lambdalisue Thanks for your contribution!

@kt3k kt3k merged commit 987fc72 into denoland:main Feb 25, 2024
12 checks passed
@lambdalisue lambdalisue deleted the collections-pick-omit branch February 25, 2024 12:45
eryue0220 added a commit to eryue0220/deno_std that referenced this pull request Feb 28, 2024
@eryue0220
Merge branch 'fix/expect-custom-equality-case' of github.com:eryue022…
810250f 
…0/deno_std into fix/expect-custom-equality-case

* 'fix/expect-custom-equality-case' of github.com:eryue0220/deno_std: (63 commits)
  docs: link to `assertThrows()` and `assertRejects()` (denoland#4395)
  chore(log): sync `level` and `levelName` in BaseHandler (denoland#4393)
  docs: ignore bad snippet examples (denoland#4388)
  chore(media_types): format test names (denoland#4380)
  docs: clarify underscore guidance in README (denoland#4385)
  feat(collections): add `pick` and `omit` (denoland#4218)
  chore(msgpack): format test names (denoland#4381)
  refactor(encoding): prepare for `noUncheckedIndexedAccess` (denoland#4275)
  refactor(streams): prepare for `noUncheckedIndexedAccess` (denoland#4377)
  chore: fix .editorconfig syntax (denoland#4376)
  chore(semver): remove legacy `Range.ranges` object definition (denoland#4374)
  chore(semver): move breaking versions (denoland#4372)
  refactor(semver): rename `comparatorFormat()` to `formatComparator()` (denoland#4373)
  test(semver): add test for parse_range (denoland#4345)
  chore: use 'release' event for triggering jsr publish (denoland#4370)
  chore(http): fix spawned tests after migration script (denoland#4368)
  chore(crypto): move test scripts to own files (denoland#4367)
  0.217.0 (denoland#4369)
  build: update _ to - in workspace converter script (denoland#4357)
  chore(media_types): move `extensions` utility (denoland#4358)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iuioiua iuioiua iuioiua approved these changes

@kt3k kt3k kt3k approved these changes

@petamoriken petamoriken Awaiting requested review from petamoriken

Assignees
No one assigned
Labels
collections feedback welcome We want community's feedback on this issue or PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@lambdalisue @timreichen @kt3k @Leokuma @petamoriken @iuioiua