add support for a types array #2410
Conversation
|
@domoritz , what do you think of this? I haven't yet run it locally outside of tests, so I'm not sure if it's hooked up to the CLI etc. |
|
Also, it should probably throw if you try to pass both |
domoritz
left a comment
domoritz
left a comment
There was a problem hiding this comment.
That's great. Should we simplify the internal apis to only accept types and then once at the beginning convert to a list of the user provides type via the api?
|
What is the process I should use for running my version locally? I want to convince myself it works. |
|
Test and cli can run locally. See readme for instructions. |
| * Use "*" to generate schemas for all exported types. | ||
| */ | ||
| type?: string; | ||
| type?: string | string[]; |
There was a problem hiding this comment.
I'm not positive this is the right way to type this.
There was a problem hiding this comment.
Because of how you set a (value: string, previous: string[] | undefined) mapper, wouldn't type always be an array? Most likely an array with a single item inside?
There was a problem hiding this comment.
If so, i think you can simplify the code by removing the castArray and letting all related types be a single array[] (Also remove undefined support and just check for types.length === 0.
There was a problem hiding this comment.
If not, cast it into an array at the createGenerator(config).createSchema(config.type) level.
There was a problem hiding this comment.
I wonder if I'm misunderstanding what you guys are asking.
I thought that this Config object was defining valid inputs for the library, so that by type?: string | string[], we were saying that programmatic usage accepts undefined, a single string, or an array of string.
And I thought the (value: string, previous: string[] | undefined) mapper was only applying when invoked via the CLI. If --type is not passed on the CLI, the final value is undefined (b/c the mapper is never invoked). If one or more values are passed, then mapper is invoked and it will always be an array, even with just a single item.
So I think if Config.type was changed to an array, I think it would be a breaking change.
I have a PR making this change here against an older version of this branch:
Am I misunderstanding something? Is that version basically the change you're asking for? Happy to do it if so!
There was a problem hiding this comment.
I think supporting both and just using the default is easier. I'll make the change.
Tests are passing locally, and using the CLI locally seemed to do what I wanted it to do. |
| .option( | ||
| "-t, --type <name>", | ||
| "Type name (can be passed multiple times)", | ||
| (value: string, previous: string[] | undefined) => { |
There was a problem hiding this comment.
if it will always return an array, then the typings can be a simple string[]
There was a problem hiding this comment.
My thinking here was that previous can be undefined, since I'm not passing a default arg.
There was a problem hiding this comment.
Shouldn't we use ... instead as documented in https://github.com/tj/commander.js?
There was a problem hiding this comment.
My understanding (and this is just from reading the docs and talking to chatgpt--I don't have any real experience with commander), was that ... allows for --type Foo Bar rather than --type Foo --type Bar, as Arthur asked for (and that I guess is more common, at least according to the LLMs). There were also some parsing ambiguities around variadic arguments that seemed to trip people up when I googled.
Although I see now that one of their examples does show both working:
$ collect --letter -n 1 -n 2 3 -- operand
Options: { number: [ '1', '2', '3' ], letter: true }
Remaining arguments: [ 'operand' ]
They have a section in the docs about ambiguity in parsing variadic arguments that I didn't think applied to the collect approach I took here.
Let me know if you'd prefer I switch to variadic arguments.
There was a problem hiding this comment.
I like his approach of always returning an array :)
domoritz
left a comment
domoritz
left a comment
There was a problem hiding this comment.
Looks good once the last comment is resolved.
|
Could you fix the lint error so we can merge? |
…a-generator into sam-multiple-types
Head branch was pushed to by a user without write access
Done, I think. |
Added a test to try and get the patch coverage to pass. I think the only line that I added that wasn't tested was throwing for mixed types, like |
| .option( | ||
| "-t, --type <name>", | ||
| "Type name (can be passed multiple times)", | ||
| (value: string, previous: string[] | undefined) => { |
There was a problem hiding this comment.
I like his approach of always returning an array :)
| * Use "*" to generate schemas for all exported types. | ||
| */ | ||
| type?: string; | ||
| type?: string | string[]; |
There was a problem hiding this comment.
Because of how you set a (value: string, previous: string[] | undefined) mapper, wouldn't type always be an array? Most likely an array with a single item inside?
| * Use "*" to generate schemas for all exported types. | ||
| */ | ||
| type?: string; | ||
| type?: string | string[]; |
There was a problem hiding this comment.
If so, i think you can simplify the code by removing the castArray and letting all related types be a single array[] (Also remove undefined support and just check for types.length === 0.
| * Use "*" to generate schemas for all exported types. | ||
| */ | ||
| type?: string; | ||
| type?: string | string[]; |
There was a problem hiding this comment.
If not, cast it into an array at the createGenerator(config).createSchema(config.type) level.
|
🚀 PR was released in |
|
We need |
3 participants
Overview
The tool currently supports either generating types for all exported types (which you get by not passing the
typeor passing'*'). There is no way currently to emit only a list of named types. So you wind up with either a single type or every exported type. This is requested in #1223.This PR adds a
--typesproperty that accepts an array of type names. This lets you generate a list of types.In this implementation, types wind up at the top-level even if they are referenced but not listed in the types array. So this, eg:
With invoked as
--types=MyObject1,MyObject2will include to-level types forMyObject1,MyObject2, ANDObject1Prop.Object2Propwill be inlined.Version
Published prerelease version:
v2.5.0-next.8Changelog
🎉 This release contains work from new contributors! 🎉
Thanks for all your work!
❤️ Sam Sudar (@srsudar)
❤️ Orta Therox (@orta)
❤️ James Vaughan (@jamesbvaughan)
❤️ Alex (@alexchexes)
❤️ Cal (@CalLavicka)
❤️ Valentyne Stigloher (@pixunil)
🚀 Enhancement
NewExpressionparser #2346 (@jamesbvaughan)🐛 Bug Fix
--additional-propertiesoption #2305 (@alexchexes)--type "*"is used with multiple exports #2284 (@alexchexes @arthurfiorette)symbol#2282 (@alexchexes)🔩 Dependency Updates
Authors: 9