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

add {Type}LongVar and {Type}ShortVar helpers #134

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

Conversation

lopezator
Copy link

Add helper functions to avoid repeating the 0 and/or "" in the case of Var flagset method calls.

@lopezator
Copy link
Author

lopezator commented Apr 9, 2024

I tend to use just long flag names for all my applications, so moving out from flag.FlagSet to ff.FlagSet means in my case having to add a 0 to every line, like:

fs.StringVar(&cfg.GRPCAddr, 0, "grpc-addr", "localhost:8000", "gRPC address")
fs.StringVar(&cfg.HTTPAddr, 0, "http-addr", "localhost:8001", "HTTP address")
// And so on...

As StringShort and StringLong were already present, I took the opportunity to add the short method as well, so others can benefit from it. It follows the same approach as the mentioned ones, just porting the functionality to the Var methods.

I suppose you didn't add this before to avoid this implementation for every type, but in my case at least, string is like the 90% of the the flags.

If you feel that we should add support for all types, I can do that as well. Just wanted to start small and simple.

Thanks for the wonderful library btw @peterbourgon

@peterbourgon
Copy link
Owner

So I'm not totally convinced that the benefit from expanding the API surface area to include these new methods outweighs the cost of adding these zero-value params, but I guess it could make sense in a holistic way. Can you add the same variants to the other type-specific XxxVar methods on FlagSet?

@lopezator lopezator changed the title add StringLongVar and StringShortVar helpers add {Type}LongVar and {Type}ShortVar helpers Apr 10, 2024
@lopezator
Copy link
Author

lopezator commented Apr 10, 2024

So I'm not totally convinced that the benefit from expanding the API surface area to include these new methods outweighs the cost of adding these zero-value params

To be honest, I felt the same way, but as the concession is already made on the flagSet types without var (fs.String, fs.Bool...) I thought it might make sense adding the xxxVar variants as well for the sake of consistency.

Another option could be to remove all the Short and Long variants everywhere? But I guess that would require more work and also breaks the API, but it's maybe acceptable since this is not a stable release yet, I don't know 🙂

I added the rest of the types along with two new tests.

Thanks a lot for the quick review and let me know if I can help in another way.

@lopezator lopezator force-pushed the feature/string-short-long-var branch 3 times, most recently from ddf2389 to 809ebbd Compare April 10, 2024 16:16
Add helper functions to avoid repeating the `0` and/or `""`
in the case of `Var` flagset method calls.
@lopezator lopezator force-pushed the feature/string-short-long-var branch from 809ebbd to f039960 Compare April 10, 2024 16:19
@lopezator
Copy link
Author

No hurries, obviously, but if there is there anything else I can improve on my end, just tell me @peterbourgon .

Thanks!

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.

None yet

2 participants