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

New: swap #42

Merged
merged 2 commits into from
Jun 23, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,14 @@

1. Run `npm install` to install needed local dependencies.

1. Run `npm run build` first, then `npm run test` and address any errors. Preferably, fix commits in place
1. Run `npm run build` first, then `npm run test` and `npm run lint` and address any errors. Preferably, fix commits in place
using `git rebase` or `git commit --amend` to make the changes easier to
review and to keep the history tidy.

1. Push to your fork:

$ git push origin <branch>

2. Open a pull request.
1. Open a pull request.

TODO: Explain the convention for creating types for ramda
30 changes: 30 additions & 0 deletions test/swap.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
import { expectType } from 'tsd';
import { __, pipe, compose, swap } from '../es';

const indexA = 0;
const indexB = 1;
const list: number[] = [];

expectType<number[]>(swap(indexA, indexB, list));

expectType<number[]>(swap(__, indexB, list)(indexA));

expectType<number[]>(swap(indexA, __, list)(indexB));

expectType<number[]>(swap(__, __, list)(indexA, indexB));
expectType<number[]>(swap(__, __, list)(__, indexB)(indexA));
expectType<number[]>(swap(__, __, list)(indexA)(indexB));

expectType<number[]>(swap(indexA, indexB)(list));

expectType<number[]>(swap(__, indexB)(indexA, list));
expectType<number[]>(swap(__, indexB)(__, list)(indexA));
expectType<number[]>(swap(__, indexB)(indexA)(list));

expectType<number[]>(swap(indexA)(indexB, list));
expectType<number[]>(swap(indexA)(__, list)(indexB));
expectType<number[]>(swap(indexA)(indexB)(list));

// sanity checks for compose and pipe
expectType<(list: readonly number[]) => number[]>(pipe(swap(indexA, indexB)));
expectType<(list: readonly number[]) => number[]>(compose(swap(indexA, indexB)));
58 changes: 58 additions & 0 deletions types/swap.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
import { Placeholder } from './util/tools';

// swap(indexA)
Copy link
Contributor

Choose a reason for hiding this comment

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

I really like the comments! Makes all these overloads much easier to follow 👍🏻

export function swap(indexA: number): {
// swap(indexA)(indexB)(list)
(indexB: number): <T>(list: readonly T[]) => T[];
// swap(indexA)(__, list)(indexB)
<T>(__: Placeholder, list: readonly T[]): (indexB: number) => T[];
// swap(indexA)(indexB, list)
<T>(indexB: number, list: readonly T[]): T[];
};

// swap(__, indexB)
export function swap(
__: Placeholder,
indexB: number
): {
// swap(__, indexB)(indexA)(list)
(indexA: number): <T>(list: readonly T[]) => T[];
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious about the readonly keyword here. Is this just preventing the definition of swap from mutating the list argument? I guess the implementation of swap is already written in the other repo and these definitions do not restrict that implementation.

Does readonly do anything else that restricts the consumer or type inference in any way? It probably can't hurt to have these here, but just wanted to see if I was missing something else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're spot-on, but I wouldn't describe it as "only" doing what it does because I think it's a pretty powerful TS feature.

If you have a readonly array and a function that takes an array, you have no guarantee that the function won't modify the array. Thus, TS won't let you pass your array into the function. If you make the argument readonly, then you're telling the caller that you won't modify it. Plus, a non-readonly array basically extends a readonly array, so it's also a valid argument to the function. So slapping a readonly on your parameter means you can accept any argument, readonly or not.

Every function in Ramda is a pure function so there should be readonlys throughout, which just means that you can call them with whatever you want. I've run into really annoying libraries that don't include the readonly even though they actually are. If you have a dropdown component, then define const DROPDOWN_OPTIONS = [...] as const, if the dropdown typing doesn't have the options as readonly you won't be able to pass that in.

Copy link
Contributor

Choose a reason for hiding this comment

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

ahh, that makes sense. It does help the consumer in that this function now accepts readonly arrays. Thanks for the explanation!

Copy link
Collaborator

Choose a reason for hiding this comment

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

// swap(__, indexB)(__, list)(indexA)
<T>(__2: Placeholder, list: readonly T[]): (indexA: number) => T[];
// swap(__, indexB)(indexA, list)
<T>(indexA: number, list: readonly T[]): T[];
};

// swap(indexA, indexB)(list)
export function swap(indexA: number, indexB: number): <T>(list: readonly T[]) => T[];

// swap(__, __, list)
export function swap<T>(
__: Placeholder,
__2: Placeholder,
list: readonly T[]
): {
// swap(__, __, list)(indexA)(indexB)
(indexA: number): (indexB: number) => T[];
// swap(__, __, list)(__, indexB)(indexA)
(__3: Placeholder, indexB: number): (indexA: number) => T[];
// swap(__, __, list)(indexA, indexB)
(indexA: number, indexB: number): T[];
};

// swap(indexA, __, list)(indexB)
export function swap<T>(
indexA: number,
__: Placeholder,
list: readonly T[]
): (indexB: number) => T[];

// swap(__, indexB, list)(indexA)
export function swap<T>(
__: Placeholder,
indexB: number,
list: readonly T[]
): (indexA: number) => T[];

// swap(indexA, indexB, list)
export function swap<T>(indexA: number, indexB: number, list: readonly T[]): T[];