-
Notifications
You must be signed in to change notification settings - Fork 16
feat: adds ability to access original flag keys post-generation #167
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
base: main
Are you sure you want to change the base?
feat: adds ability to access original flag keys post-generation #167
Conversation
0ec59d2
to
6b428d1
Compare
Signed-off-by: Kris Coleman <[email protected]>
Adds a help target to the Makefile. This target provides a list of available commands and their descriptions, improving the user experience when interacting with the Makefile. Signed-off-by: Kris Coleman <[email protected]>
6b428d1
to
747f745
Compare
GitHub Actions uses Dagger v0.19.0 while locally we were using v0.18.12, causing API incompatibility errors in integration tests. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]> Signed-off-by: Kris Coleman <[email protected]>
dfb5100
to
5d22130
Compare
Adds automatically generated openfeature integration for go. This keeps the integration tests in-sync. Also includes golang 1.24 upgrades Signed-off-by: Kris Coleman <[email protected]>
def5d0a
to
1930e7f
Compare
@kriscoleman I will hopefully be able to look over this more tomorrow, my computer is having issues and I am having to use a different one but it means setting up some of the basics to look over things like this. 😅 GitHub isn't the best way to read over code for me but looking at it nothing jumped out at me and from what I can tell it does touch on all the points raised in proposed issue. |
/gemini review |
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.
Code Review
This pull request introduces a valuable feature by enabling programmatic access to the original flag keys after code generation, implementing this for all supported languages. The approach is idiomatic for each language, using fmt.Stringer
in Go and FlagKeys
constants in others. The changes are well-structured, and the Go integration tests have been updated to validate the new functionality. My main feedback is to ensure the C# integration tests are also updated to cover this new feature, as the provided generated file for the test seems to be missing the FlagKeys
class.
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.
It works great except for when the description is omitted from the flag manifest. Please add support for the scenario and I'll approve. Thanks!
@beeme1mr I fixed the missing flag keys from the csharp .g.cs and also added logic to handle missing descriptions in the generated comments in the clients. I'm a little uncertain what to use when the descriptions are omitted, so I went with "Flag Key for this flag" in the event the flag description isn't there. |
61730e5
to
496fac8
Compare
That seems like a good approach. We can always change it later since it won't be a breaking change. |
/gemini review |
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.
Code Review
This pull request introduces a valuable feature, enabling developers to access original flag keys from the generated typed clients. The implementation is well-executed and idiomatic for each of the supported languages, using fmt.Stringer
in Go and FlagKeys
constants in others. The changes are comprehensive, covering templates, golden test files, and integration tests. I've identified a couple of minor areas for improvement concerning formatting consistency in the generated Java code and comment consistency in the Python template to enhance the overall quality.
*/ | ||
FlagEvaluationDetails<Double> discountPercentageDetails(EvaluationContext ctx); | ||
|
||
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 blank line now contains spaces, which is a minor formatting regression. This issue is repeated for all generated methods in this file. Please adjust the template to output a truly blank line for better code formatting and consistency. This can be fixed by adding a newline before the {{ end }}
statement in the loops within internal/generators/java/java.tmpl
.
class FlagKeys: | ||
"""Flag key constants for programmatic access""" | ||
{{- range .Flagset.Flags }} | ||
{{ .Key | ToScreamingSnake }} = {{ .Key | Quote }} # {{ if .Description }}{{ .Description }}{{ else }}Feature flag{{ end }} |
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.
For consistency with the other language generators in this PR (e.g., C#, Java, JS), the comment for the flag key constant should be more descriptive. Consider prefixing it with 'Flag key for: ' and using 'this flag' as the fallback for a missing description to align it with other implementations.
{{ .Key | ToScreamingSnake }} = {{ .Key | Quote }} # Flag key for: {{ if .Description }}{{ .Description }}{{ else }}this flag{{ end }}
… descriptions Signed-off-by: Kris Coleman <[email protected]>
496fac8
to
262191d
Compare
This PR
Adds the ability to access original flag keys post-generation by implementing
fmt.Stringer
interface on generated flag structs.fmt.Stringer
interface for all generated flag structs across all supported languagesstringer
type and helper in Go templates to return original flag keysFlagKeys
constants class in C# templates for programmatic access to flag keys.String()
methodRelated Issues
Closes #109
Notes
This feature allows developers to retrieve the original flag key string from generated flag objects, which is particularly useful for:
The implementation uses Go's
fmt.Stringer
interface pattern and similar approaches in other languages to provide a clean API.How to test
.String()
method (Go) or accessFlagKeys
constants (C#)cd test/go-integration && go test