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

Ensure primary library usage is via struct literals #1774

Open
Tracked by #833
meatballhat opened this issue Jun 21, 2023 · 3 comments
Open
Tracked by #833

Ensure primary library usage is via struct literals #1774

meatballhat opened this issue Jun 21, 2023 · 3 comments
Labels
area/v3 relates to / is being considered for v3 kind/cleanup describes internal cleanup / maintaince
Milestone

Comments

@meatballhat
Copy link
Member

meatballhat commented Jun 21, 2023

One of the strongest differentiators of this library is (arguably! (I'm arguing)) how declarative it is. Ensuring the primary usage of the library is via struct literals will support this differentiation, imho.

In practice, this can certainly become awkward to the point of being a bad user experience. For example, consider the "value source" implementation Sources: EnvVars(...) that replaces the v2 EnvVars: []string{...}. The EnvVars function is not a literal declaration of a type, but the usage is expected to be within a literal declaration of a Flag type.

Judgement calls must be made!

@meatballhat meatballhat added this to the Release 3.x milestone Jun 21, 2023
@meatballhat meatballhat added the area/v3 relates to / is being considered for v3 label Apr 27, 2024
@bartekpacia
Copy link
Member

Just to make sure I understand it correctly:

  • Sources: EnvVars(...) is something we don't want because it's using a function call, not a struct literal.
  • The way it was done in v2 (with EnvVars: []string{...}) was declarative, this good for us

If I understood this correctly, then I agree with it.

@bartekpacia bartekpacia added the kind/cleanup describes internal cleanup / maintaince label May 3, 2024
@meatballhat
Copy link
Member Author

@bartekpacia The example you're giving is actually one that I'm not sure about 😅

In the v1 API, there were a few "stringly typed" fields that would be processed with strings.Split("a,b", ",") and strings.TrimSpace, which resulted in some annoying and bug-inducing patterns like having to process the strings in many places.

In v2 we made sure to require []string{"a", "b"} instead of "a,b" and added fields like Aliases to allow Name to remain a string. We also saw a lot of activity in ./altsrc that contributed to more awkward dependency management issues. I mention this because it's related to the existence of the v3 Sources field.

In v3, I am hoping we can accommodate environment variables, files of various formats, and whatever else we haven't thought of, all via the Sources field on each flag type. The inclusion of environment variables in this goal means that accepting something more v2-ish like EnvVars: []string{...} introduces other tradeoffs around precedence, merging, egad, and I'm currently favoring trying to use only the Sources field.

So! I don't want to be too strict about any of these rules, whatever that may mean at the time. I hope that we can lean more heavily into the "declarative" differentiator of this library. I don't want to reintroduce a separate EnvVars field if we can help it, so I think this means that I'm favoring a smaller API surface area over being declarative everywhere.

WDYT?

@dearchap
Copy link
Contributor

Is this still an issue ? We've fixed ValueSourceChain so that sources can be defined as

    Sources: cli.EnvVars("a", "b", c"),

or

    Sources: cli.ValueSourceChain{
       Chain: []cli.ValueSource {
            cli.EnvVar("a"),
            cli.EnvVar("b"),
            cli.EnvVar("c"),

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/v3 relates to / is being considered for v3 kind/cleanup describes internal cleanup / maintaince
Projects
None yet
Development

No branches or pull requests

3 participants