- Notifications
You must be signed in to change notification settings - Fork1k
chore: support external types in typescript codegen#9633
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| // The code below is generated from cli/clibase. | ||
| // From clibase/clibase.go | ||
| exporttypeClibaseAnnotations=Record<string,string>; | ||
| // From clibase/clibase.go | ||
| exportinterfaceClibaseGroup{ | ||
| readonlyparent?:ClibaseGroup; | ||
| readonlyname?:string; | ||
| readonlyyaml?:string; | ||
| readonlydescription?:string; | ||
| } | ||
| // From clibase/option.go | ||
| exportinterfaceClibaseOption{ | ||
| readonlyname?:string; | ||
| readonlydescription?:string; | ||
| readonlyrequired?:boolean; | ||
| readonlyflag?:string; | ||
| readonlyflag_shorthand?:string; | ||
| readonlyenv?:string; | ||
| readonlyyaml?:string; | ||
| readonlydefault?:string; | ||
| // actual value is an interface that implements 'String()' | ||
| readonlyvalue?:string; | ||
| readonlyannotations?:ClibaseAnnotations; | ||
| readonlygroup?:ClibaseGroup; | ||
| readonlyuse_instead?:ClibaseOption[]; | ||
| readonlyhidden?:boolean; | ||
| readonlyvalue_source?:ClibaseValueSource; | ||
| } | ||
| // From clibase/option.go | ||
| exporttypeClibaseOptionSet=ClibaseOption[]; | ||
| // From clibase/option.go | ||
| exporttypeClibaseValueSource=""|"default"|"env"|"flag"|"yaml"; | ||
| exportconstClibaseValueSources:ClibaseValueSource[]=[ | ||
| "", | ||
| "default", | ||
| "env", | ||
| "flag", | ||
| "yaml", | ||
| ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Unfortunately, some of this code is very "bolt on", and not perfectly clean.
I think that's perfectly fine for this package! Don't let perfect be the enemy of good, and all that.
LGTM!
Totally agree, just unfortunate. Originally we thought we could open source this as a stand alone package. So every "hard coded" opinion makes that more challenging. It's obviously not a high priority, would just be cool 🤷 |
That most challenging part about making it not opinionated is the override of |
Yeah that’s true, but perhaps it’s not something worth solving by analyzing the custom marshaler. Maybe it could be solved by a separate interface that you can implement which, when implemented, returns a value (concrete type) that “looks” correct/contains all the possible values in the case of an enum. This could work with custom types within a struct as well if those too implement it. Not sure if it’s doable, just a thought I had. |
@mafredri I was having a similar thought of having the user provide the marshalled struct via some other means. Because the only way I could think of having the custom marshaller analyzed without too much hassle, was to just run the marshal with some fuzzed data and see what pops out lol. But that seems flakey and probably not perfect. |
Uh oh!
There was an error while loading.Please reload this page.
Closes#9623
We now have a lot of structs in the
clibasepackage that need supporting.Unfortunately, some of this code is very "bolt on", and not perfectly clean. A problem is that we override the
MarshalJSONfunction on some types, so those need to be handled case by case.The outputted ts file is the test imo.