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

WIP: capability to generate non Opt* struct for parameters with default value #1200

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Liooo
Copy link
Contributor

@Liooo Liooo commented Mar 19, 2024

@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.

@Liooo Liooo marked this pull request as ready for review March 19, 2024 09:34
Name string // only for struct, alias, interface, enum, stream, generic, map, sum
name string // only for struct, alias, interface, enum, stream, generic, map, sum
Copy link
Contributor Author

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
Copy link
Contributor Author

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()

Comment on lines +103 to +106
isFeatureTurnedOn := true // generate non Opt* for default valued params, or not
if isFeatureTurnedOn && o.ServerClient == ServerClientServer && t.Default().Set {
return t.name
}
Copy link
Contributor Author

@Liooo Liooo Mar 19, 2024

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

Comment on lines +189 to +196
// ServerClient tells if template method being called is for client or server side code generation
type ServerClient int

const (
ServerClientNone = iota
ServerClientClient
ServerClientServer
)
Copy link
Contributor Author

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)

Comment on lines +198 to +201
// CallerContext holds information about the caller side context of the template method being invoked
type CallerContext struct {
ServerClient
}
Copy link
Contributor Author

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 {
Copy link
Contributor Author

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.

@ernado
Copy link
Member

ernado commented Apr 4, 2024

@tdakkota PTAL
We need to decide how we going to implement this feature

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.

2 participants