-
Notifications
You must be signed in to change notification settings - Fork 889
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
// The code below is generated from cli/clibase. | ||
|
||
// From clibase/clibase.go | ||
export type ClibaseAnnotations = Record<string, string>; | ||
|
||
// From clibase/clibase.go | ||
export interface ClibaseGroup { | ||
readonly parent?: ClibaseGroup; | ||
readonly name?: string; | ||
readonly yaml?: string; | ||
readonly description?: string; | ||
} | ||
|
||
// From clibase/option.go | ||
export interface ClibaseOption { | ||
readonly name?: string; | ||
readonly description?: string; | ||
readonly required?: boolean; | ||
readonly flag?: string; | ||
readonly flag_shorthand?: string; | ||
readonly env?: string; | ||
readonly yaml?: string; | ||
readonly default?: string; | ||
// actual value is an interface that implements 'String()' | ||
readonly value?: string; | ||
readonly annotations?: ClibaseAnnotations; | ||
readonly group?: ClibaseGroup; | ||
readonly use_instead?: ClibaseOption[]; | ||
readonly hidden?: boolean; | ||
readonly value_source?: ClibaseValueSource; | ||
} | ||
|
||
// From clibase/option.go | ||
export type ClibaseOptionSet = ClibaseOption[]; | ||
|
||
// From clibase/option.go | ||
export type ClibaseValueSource = "" | "default" | "env" | "flag" | "yaml"; | ||
export const ClibaseValueSources: 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. |
Closes #9623
We now have a lot of structs in the
clibase
package that need supporting.Unfortunately, some of this code is very "bolt on", and not perfectly clean. A problem is that we override the
MarshalJSON
function on some types, so those need to be handled case by case.The outputted ts file is the test imo.