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

Consider making cueexperiment.Flags and cuedebug.Flags public #3614

Open
cuematthew opened this issue Dec 4, 2024 · 0 comments
Open

Consider making cueexperiment.Flags and cuedebug.Flags public #3614

cuematthew opened this issue Dec 4, 2024 · 0 comments

Comments

@cuematthew
Copy link
Contributor

Currently they are both internal. This has led to the creation of cuecontext.CUE_DEBUG(s string) Option which I think is unfortunate. It moves compile-time checks to runtime, and thus prevents IDE/LSP support. It also forces the user to learn the necessary string encoding. The benefit though is that it provides flexibility to us to change the Flag structs and to say that we're not making breaking changes to the public API. Given the precise behaviour of envflag.Parse, I'm not sure I buy this argument.

An alternative set of tradeoffs would be to make the two Flag structs public. This would mean they could be directly passed to cuecontext (and other) APIs, it would mean they're more discoverable, could be documented in normal ways, the user doesn't have to learn yet another string encoding, the field names are checked statically, and IDEs and LSP facilities would work. The downside, is we would very likely want to label these packages as being explicitly outside any guarantees we make about backwards compatibility / semver etc. In order to continue supporting env vars, we could have Unmarshal methods on these structs to support parsing strings.

A further embellishment: Given envflag currently only supports bools as values, the Flag structs have evolved such that every flag has only a boolean value. This has the potential for nonsensical combinations of flags such as evalv2=1,evalv3=1, when what is really desired is for the value to be the eval version desired. With this in mind, it might well make good sense for more idiomatic/regular configuration structs to be created, which are public, and can be used in the the cuecontext API. Values of these newer structs could then be built and returned by the existing Flag structs. This could allow the public API to have good, intuitive, unambiguous configuration, without simultaneously needing to make large changes to the env var "API", if that is desired.

@cuematthew cuematthew added the Triage Requires triage/attention label Dec 4, 2024
@mvdan mvdan added NeedsDecision and removed Triage Requires triage/attention labels Dec 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants