-
Notifications
You must be signed in to change notification settings - Fork 87
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
WIP: capability to generate non Opt* struct for parameters with default value #1200
base: main
Are you sure you want to change the base?
Conversation
Name string // only for struct, alias, interface, enum, stream, generic, map, sum | ||
name string // only for struct, alias, interface, enum, stream, generic, map, sum |
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.
make the field private and add an exported method Name
instead
return t.name | ||
} | ||
|
||
return t.GenericVariant.Name() + t.name |
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.
moved the only anomaly that happens for Generic()
isFeatureTurnedOn := true // generate non Opt* for default valued params, or not | ||
if isFeatureTurnedOn && o.ServerClient == ServerClientServer && t.Default().Set { | ||
return t.name | ||
} |
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.
This is where the main logic resides, if the feature is turned on and the call is made for the server side (and the param has default value), do not add Opt*
prefix. Need to figure out a way to access the feature flags here.
The reason behind adding a flag rather than making it ogen default behavior is: https://t.me/ogen_dev/9482
// ServerClient tells if template method being called is for client or server side code generation | ||
type ServerClient int | ||
|
||
const ( | ||
ServerClientNone = iota | ||
ServerClientClient | ||
ServerClientServer | ||
) |
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.
adding ServerClientNone
as a default zero value for the type, to handle any caller context that is not server nor client (not sure if there's such cases, I'll read the codebase and remove if this is not the case)
// CallerContext holds information about the caller side context of the template method being invoked | ||
type CallerContext struct { | ||
ServerClient | ||
} |
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.
Defining a struct to hold the caller context, as the same mechanism might be necessary when implementing readonly/writeonly to store the information of if the method is called for write (POST, PUT etc) or read (GET etc) operation
// Go returns valid Go type for this Type. | ||
func (t *Type) Go() string { | ||
func (t *Type) Go(opt GoOpt) string { |
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.
This strategy will require us to basically rewrite many template methods to take an argument and pass that down the call stack.
@tdakkota PTAL |
@tdakkota @ernado @shadowspore
#966
I saw some reactions on the issue and I've got some time now so diving into this.
This PR includes only a minimal draft changes to discuss the implementation strategy, once the direction is consolidated I'm happy to do the rest of the work.
Just want a quick feedback at this point about if it's looking good or there's any other better ways. Honestly, haven't given much thought nor read the whole codebase yet so there could be some limitation in this strategy.
Naming of the variables and types are also draft, will pick better ones if there's any along the way.