peterbourgon / ff

Flags-first package for configuration
Apache License 2.0
1.37k stars 59 forks source link

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

Open lopezator opened 5 months ago

lopezator commented 5 months ago

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

lopezator commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 :slightly_smiling_face:

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 commented 5 months ago

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

Thanks!