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

Generic argument support for node methods #383

Merged
merged 9 commits into from Apr 23, 2022

Conversation

golergka
Copy link
Contributor

@golergka golergka commented Apr 20, 2022

Closes #382

I'd like to add some tests that use types as well, but the relevant test collection-access.js is currently untyped, and changing it seemed a little to large a change for a first contribution.

@golergka golergka changed the title feat: generic argument support for node methods Generic argument support for node methods Apr 20, 2022
@eemeli
Copy link
Owner

eemeli commented Apr 22, 2022

@golergka I'd like to expand this PR a bit to package in a few more TS changes, including a port of collection-access.js to TypeScript. Would you be okay with me adding commits to your branch? That should allow you to also add tests for the current changes.

@golergka
Copy link
Contributor Author

@golergka I'd like to expand this PR a bit to package in a few more TS changes, including a port of collection-access.js to TypeScript. Would you be okay with me adding commits to your branch? That should allow you to also add tests for the current changes.

Certainly! It's your repo, after all 😁

src/index.ts Show resolved Hide resolved
Comment on lines +16 to +23
/** Utility type mapper */
export type NodeType<T> = T extends string | number | bigint | boolean | null
? Scalar<T>
: T extends Array<any>
? YAMLSeq<NodeType<T[number]>>
: T extends { [key: string | number]: any }
? YAMLMap<NodeType<keyof T>, NodeType<T[keyof T]>>
: Node
Copy link
Owner

Choose a reason for hiding this comment

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

This isn't currently publicly exported, but I wonder if it'd be useful outside of createNode()? Also happy to bikeshed on the name.

Copy link
Owner

@eemeli eemeli left a comment

Choose a reason for hiding this comment

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

@golergka Please re-request a review from me once you've added tests for your part and had a look at my additions? I've incorporated here changes that should resolve the issues from #381 as well.

@golergka golergka force-pushed the generic-arguments-for-node-functions branch from c2254de to c0db02a Compare April 22, 2022 17:45
src/nodes/YAMLMap.ts Show resolved Hide resolved
tests/collection-access.ts Outdated Show resolved Hide resolved
@golergka golergka requested a review from eemeli April 22, 2022 21:24
Copy link
Owner

@eemeli eemeli left a comment

Choose a reason for hiding this comment

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

I think this is getting good. A few nitpicky fixes, and then we can merge this.

src/doc/Document.ts Outdated Show resolved Hide resolved
src/nodes/Node.ts Outdated Show resolved Hide resolved
src/nodes/YAMLMap.ts Outdated Show resolved Hide resolved
src/nodes/YAMLSeq.ts Outdated Show resolved Hide resolved
tests/collection-access.ts Outdated Show resolved Hide resolved
@golergka golergka requested a review from eemeli April 23, 2022 14:44
@eemeli eemeli merged commit 133f45c into eemeli:master Apr 23, 2022
@eemeli
Copy link
Owner

eemeli commented Apr 23, 2022

Thank you @golergka! Very happy to have this included. I've a couple of other minor things that I want to get in, but then should be able to get a new release out within a week or so.

@golergka golergka deleted the generic-arguments-for-node-functions branch April 23, 2022 16:23
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.

Generic argument support for node functions
2 participants