search

peterbourgon / ff

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

ffcli: enhancements based on feedback #38

Closed peterbourgon closed 4 years ago

peterbourgon commented 5 years ago
tgulacsi commented 5 years ago

Migrating from kingpin.v2, the bits I miss to somewhat degree:

Otherwise, I like ffcli's small and clean interface!

peterbourgon commented 5 years ago

@tgulacsi Thanks for the feedback!

  • global setup when I know which command(s) are selected, and all the flags has been parsed. Only awkward workarounds found yet - but a Postparse seems Just What The Doctor Ordered!

👍

  • alias commands (can be simulated by copying and Name changing, but they don't group and all aliases appear as separate subcommnads),
  • long/short duplicate flags (flagLong := flag.String(); flagShort :=flag.StringVar(flagLong,...)) - but I don't really need this

My intuition is that I don't want to support these, because of something like there should be only one way to do things. The former case has a reasonable workaround, IMO: just use the shortest form of the name and provide a good Usage. The latter case is straight-up not possible with stdlib package flag, and I don't want to change that.

tgulacsi commented 5 years ago

Understood and accepted, thanks!

josharian commented 5 years ago

I'd like to use https://github.com/peterbourgon/ff/pull/38/commits/9a8ce55d769df22d1e09343b8e9fe7b4ee728848 now.

How do you feel about starting to land some of these commits incrementally?

peterbourgon commented 5 years ago

I'm happy to merge that bit now. Give me a moment.

peterbourgon commented 5 years ago

For the record I am unhappy with the contortions required to use Postparse successfully, and am experimenting with a two-phase Parse/Run approach for the next revision.

tgulacsi commented 5 years ago

A separate Parse and RunParsed (if Run is kept as is) steps are a good solution, IMHO.

josharian commented 5 years ago

In case it helps to have another use case as you think through the Postparse API: I'm looking for a way to hook up cpu profiling at the top level.

Postparse seems conceptually about right: once the flags etc are parsed, notice that profiling has been requested, start it, and then dispatch to subcommands as usual. It's a bit awkward in that I can't just defer stopping the profiling...but that's not a big deal, I can hard-code it at the end of func main.

A separate Parse would also work nicely for this, and let me use a defer in main the way I usually would. I don't see why you'd need a RunParsed, though; ISTM you could track parse state internally and skip it if it is already completed.

peterbourgon commented 5 years ago

Thanks for that, it's an informative use case. For the record whatever changes I make to how Run works would probably be breaking, and that's OK.

zikaeroh commented 4 years ago

Have you given these changes any more thought? I was personally hoping to use post-parse to do some flag validation (to replace what I'd lose from another library which can check for required flags if I were to switch to ffcli).

peterbourgon commented 4 years ago

I think I've convinced myself that I need a two-phase Parse + Run API instead of the Run + Postparse approach currently in the branch, but I haven't had the time to develop the implementation. I should find some time in the coming weeks.

peterbourgon commented 4 years ago

OK, I think package ffcli is about where I wanted to get it. The examples could be expanded, but in the interest of shipping, would those of you watching the PR please take a look at things, and tell me what you think?

SvenDowideit commented 4 years ago

I think it would be excellent to call out explicitly in the README that you have no intention/desire to change FlagSet, and so will not support aliases like -t and --tty, or option combining like -it.

I played a little with your module, and yup, its nice, I like it, but as I (think I) need to support options that mimic an existing tool that does that I was going to make an issue asking about it - and then I read this PR :)