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

go: generate *Ctx version of public functions #457

Merged
merged 1 commit into from
Sep 23, 2024

Conversation

passichenko
Copy link
Contributor

@passichenko passichenko commented Sep 20, 2024

OTel integration requires passing contexts down into lekko client. It's a common practice in Go to have special versions of functions that take context, so with this change we will generate two public getters: GetFoo() and GetFooCtx(ctx).

Copy link
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @passichenko and the rest of your teammates on Graphite Graphite

@passichenko passichenko marked this pull request as ready for review September 20, 2024 22:46
@@ -684,6 +693,10 @@ func (g *goGenerator) genGoForFeature(ctx context.Context, r repo.ConfigurationR
if err := templ.Execute(&ret, data); err != nil {
return nil, err
}
data.PassCtx = true
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 is ugly and relies on the fact that we don't use PassCtx in private functions
but it was the simplest way to reuse the template

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: a comment here to explain the reasoning behind doing this twice would be helpful

@@ -401,19 +402,22 @@ func (c *LekkoClient) {{$.FuncName}}({{$.ArgumentString}}) {{$.RetType}} {
}
debug.LogDebug("Lekko fallback", "name", "{{$.Namespace}}/{{$.Key}}", "result", result)
return result
}`,
}
`,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here and below: this is to have a newline after each function

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought gofmt should handle this automatically 🤔 but don't mind this change regardless

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in this case the issue was that without a newline the description of the context-aware function was on the same line as the closing } and gofmt can't fix that

@passichenko passichenko force-pushed the 09-20-go_generate_ctx_version_of_public_functions branch from df08ed7 to cc33fff Compare September 20, 2024 23:46
@@ -401,19 +402,22 @@ func (c *LekkoClient) {{$.FuncName}}({{$.ArgumentString}}) {{$.RetType}} {
}
debug.LogDebug("Lekko fallback", "name", "{{$.Namespace}}/{{$.Key}}", "result", result)
return result
}`,
}
`,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought gofmt should handle this automatically 🤔 but don't mind this change regardless

@@ -684,6 +693,10 @@ func (g *goGenerator) genGoForFeature(ctx context.Context, r repo.ConfigurationR
if err := templ.Execute(&ret, data); err != nil {
return nil, err
}
data.PassCtx = true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: a comment here to explain the reasoning behind doing this twice would be helpful

@passichenko passichenko force-pushed the 09-20-go_generate_ctx_version_of_public_functions branch from cc33fff to 5bc5295 Compare September 23, 2024 16:51
@passichenko passichenko merged commit 44354c9 into main Sep 23, 2024
2 checks passed
@passichenko passichenko deleted the 09-20-go_generate_ctx_version_of_public_functions branch September 23, 2024 17:17
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