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

filter over Map #136

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

filter over Map #136

wants to merge 1 commit into from

Conversation

davetapley
Copy link

@@ -10,11 +10,16 @@ export function filter<A, P extends A>(
<B extends A>(list: readonly B[]): P[];
};

// filter(() => boolean)
// filter(() => boolean, list)
Copy link
Collaborator

Choose a reason for hiding this comment

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

// filter(() => boolean) was correct as is. More accurately it's // filter(() => boolean)(list | dict)

@davetapley
Copy link
Author

@Harris-Miller thanks for feedback, fixed and rebased

@@ -10,10 +10,18 @@ export function filter<A, P extends A>(
<B extends A>(list: readonly B[]): P[];
};

// filter(() => boolean)
// filter(() => boolean)(list | dict)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// filter(() => boolean)(list | dict)
// filter(() => boolean)

Comment on lines +17 to +18
<K>(map: Map<K, T>): Map<K, T>;
<P extends T, C extends readonly P[] | Record<string, P>>(collection: C): C;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
<K>(map: Map<K, T>): Map<K, T>;
<P extends T, C extends readonly P[] | Record<string, P>>(collection: C): C;
// filter(() => boolean)(map)
<K>(map: Map<K, T>): Map<K, T>;
// filter(() => boolean)(list | dict)
<P extends T, C extends readonly P[] | Record<string, P>>(collection: C): C;

@@ -10,10 +10,18 @@ export function filter<A, P extends A>(
<B extends A>(list: readonly B[]): P[];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Github doesn't let you add suggests to unchanged code, but you need to add type support for Map here too. This whole things should be:

// filter(() => narrow)
export function filter<A, P extends A>(
  pred: (val: A) => val is P,
): {
  // filter(() => narrow)(map)
  <K, B extends A>(map: Map<K, B>): Map<K, P>;
  // if we put `Record<string, A>` first, it will actually pic`A[]` as well
  // so it needs to go first
  // filter(() => narrow)(list)
  <B extends A>(list: readonly B[]): P[];
  // filter(() => narrow)(dict)
  <B extends A>(dict: Record<string, B>): Record<string, P>;
  // but we also want `A[]` to be the default when doing `pipe(filter(fn))`, so it also needs to be last
  // filter(() => narrow)(list | dict)
  <B extends A>(list: readonly B[]): P[];
};

Comment on lines +95 to +97
// filter(() => narrow)(dist)
expectType<Map<string, string>>(filter(isNotNil, new Map<string, string | undefined>()));
expectType<Map<string, number>>(filter(gt5, new Map<string, number>()));
Copy link
Collaborator

Choose a reason for hiding this comment

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

First, these tests are for // filter(() => narrow, map)

Second, you need to be far more exhaustive with your test additions. This file has test coverage for all curry varieties and argument type overloads. You added support for a new argument type, so you need to add tests for each curry call variation

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.

2 participants