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

Allow record/map to transform type with optional properties #60

Merged
merged 11 commits into from
Aug 23, 2022

Conversation

baoshan
Copy link
Collaborator

@baoshan baoshan commented Jun 3, 2022

The current implementation of record/map refuses to map a type with optional properties:

import * as R from "./record.ts";
type Foo = { bar: number; baz?: number };
const foo: Foo = { bar: 1 };
R.map((n: number) => n + 1)(foo);
                            ^^^

Because:

Argument of type 'Foo' is not assignable to parameter of type '{ bar: number; baz: number; }'.
  Property 'baz' is optional in type 'Foo' but required in type '{ bar: number; baz: number; }'

This PR allows such usage.

@baoshan
Copy link
Collaborator Author

baoshan commented Jul 18, 2022

Any chance this PR could have a review? Thanks.

record.ts Outdated Show resolved Hide resolved
@pixeleet
Copy link
Collaborator

@baoshan Thank you for your contribution!
Apologies for the non-timely review, we're in the cucumber season.

There's one non-blocking nitpick that we should discuss with @baetheus, but otherwise, LGTM.

pixeleet
pixeleet previously approved these changes Jul 18, 2022
Copy link
Collaborator

@pixeleet pixeleet left a comment

Choose a reason for hiding this comment

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

LGTM

pending 1 non-blocking discussion/nitpick.

@baoshan
Copy link
Collaborator Author

baoshan commented Jul 18, 2022

Thanks for the review.

Switched to fenced ts block for now. Feel free to let me know if you want some hands to port existing examples to any chosen style.

baetheus
baetheus previously approved these changes Aug 23, 2022
@baetheus
Copy link
Owner

This looks good, just needs another deno fmt and I'll merge it right in.

@baetheus baetheus merged commit 95d763f into baetheus:main Aug 23, 2022
baetheus pushed a commit that referenced this pull request Oct 11, 2022
* refactor!: curried record/omit with immutable keys
* fix(record/map): allow optional properties
* fix(fmt): re-format using latest version of deno
* fix: fenced ts block instead of @example tag

Co-authored-by: Brandon Blaylock <[email protected]>

FossilOrigin-Name: 939d8c2cde7a8c9138b3770a2c3fea2bb76488d9c5fd287f1d7612e18bf724e4
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