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: add sort shorthand for sorting by property name #36

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

aleclarson
Copy link
Member

@aleclarson aleclarson commented Jun 27, 2024

Tip

The owner of this PR can publish a preview release by commenting /publish in this PR. Afterwards, anyone can try it out by running pnpm add radashi@pr<PR_NUMBER>.

Description

sort(array, 'key')
// ^ identical to…
sort(array, obj => obj.key)

The property name may be a string, number, or symbol.

This PR will affect the following functions:

  • sort
  • alphabetical
  • min
  • max

Depends on #34, #35, #43

Checklist

  • Changes are covered by tests if behavior has been changed or added
  • Tests have 100% coverage
  • If code changes were made, the documentation (in the /docs directory) has been updated

Resolves

@aleclarson aleclarson force-pushed the feat/sort-property-shorthand branch from bfb2542 to cbb3fab Compare June 27, 2024 21:45
@aleclarson aleclarson mentioned this pull request Jun 28, 2024
2 tasks
@aleclarson aleclarson force-pushed the feat/sort-property-shorthand branch from cbb3fab to 67a6fc2 Compare June 28, 2024 01:52
@aleclarson aleclarson added the new feature This PR adds a new function or extends an existing one label Jun 28, 2024

export type Comparable =
| number
| string
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if leftValue > rightValue ? 1 : leftValue < rightValue ? -1 : 0 is a good comparator for strings. We can use localCompare or not provide a default comparator for strings at all.

Copy link
Member Author

@aleclarson aleclarson Jul 1, 2024

Choose a reason for hiding this comment

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

The default comparator is intended to be both minimal and flexible (support comparing any two Comparable values). If only strings are being compared, the caller can opt-in to localeCompare like alphabetical is doing.

I suppose we could include a warning about this behavior in the function's description.

Comment on lines +24 to +26
: by !== undefined
? (arg: T) => arg[by as keyof T] as U
: (arg: T) => arg as unknown as U
Copy link
Member

Choose a reason for hiding this comment

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

It's possible to use undefined as an object key. Yes, such code is very weird, but it's still possible. So returning identity function here could be a surprising behavior.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, we could document this behavior.

Property names are assumed to be strings, numbers, or symbols.

@aleclarson aleclarson force-pushed the main branch 3 times, most recently from 35fc67a to 04bad7b Compare July 1, 2024 19:17
@aleclarson aleclarson force-pushed the main branch 2 times, most recently from 2154f96 to 6a4b4f6 Compare July 12, 2024 00:23
@aleclarson aleclarson added the stage 0: proposed A proposal for a change that is offered for community and team evaluation. label Sep 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature This PR adds a new function or extends an existing one stage 0: proposed A proposal for a change that is offered for community and team evaluation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants