optparse-applicative icon indicating copy to clipboard operation
optparse-applicative copied to clipboard

Add --version parser

Open m1-s opened this issue 1 year ago • 5 comments

This MR contains a new InfoMod that allows to specify a version which can be requested with -v or --version.

Open questions:

  • Is version a too generic name? Maybe appVersion is better?
  • Does this need more documentation somewhere?
  • Maybe not occupy -v as it may be used differently by some apps?

m1-s avatar Aug 05 '22 16:08 m1-s

@HuwCampbell whats your opinion on this?

m1-s avatar Aug 08 '22 07:08 m1-s

@HuwCampbell i am also very interested in this feature as this is something that is duplicated practically across all apps ever.

tfc avatar Aug 08 '22 09:08 tfc

Thanks for opening this conversation.

So, obviously a lot of people add these, and really it's not that hard, but there are a few subtleties which I discuss in the wiki.

The way you've used an InfoMod is kind of nice, but out of step with helper. Maybe we could migrate to the helper function being just included with a modifier to not? Until then it's a bit janky to do things both ways.

Here's what I suggest in the wiki:

versioner :: Parser (a -> a)
versioner = infoOption (showVersion version) (long "version" <> help "Show version" <> hidden)

opts :: Parser Command
opts = info (base <**> helper <**> versioner) idm

As to your specific questions:

a) Yes, clearly, as naming it version even broke the test suite, and hundreds of people having to now do qualified imports and be annoyed at me would not be fun (and I would feel bad for making them do extra work). b) The wiki has some. Otherwise great haddocks and examples in the readme go a long way. c) Tough call, and one of the reasons I haven't just added this already. -v is either "version", or "verbose" in standard conventionl while -h is almost universally "help". So one might want to make this an argument to the function.

The silly thing though is that as soon as we start adding customisations to versioner, we're almost writing the whole thing anyway!

I'll think about this more.

HuwCampbell avatar Aug 08 '22 12:08 HuwCampbell

I'll think about this more.

Ok let me know once you have made a decision. In case you care about my opinion on this:

The way you've used an InfoMod is kind of nice, but out of step with helper. Maybe we could migrate to the helper function being just included with a modifier to not? Until then it's a bit janky to do things both ways.

To summarize your statement: Either we go with base <**> helper <**> versioner (and potentially provide versioner as part of the library) or we get rid of the base <**> ... pattern by making helper a default and then use the InfoMod way for the versioner? I would prefer the 2nd option because it simplifies the API, leads to less code and provides good defaults for the user.

a) Yes, clearly, as naming it version even broke the test suite

We could call it cliAppVersion then to avoid ambiguity.

The silly thing though is that as soon as we start adding customisations to versioner, we're almost writing the whole thing anyway!

Thats true. Providing customization options does not make sense for such a simple thing. This should not keep us from providing a good default that satisfies the needs of most people though. This default probably means using only --version and nothing else. If people don't like it they may write their own version (pun intended).

m1-s avatar Aug 08 '22 15:08 m1-s

@HuwCampbell Have you had time to make your mind up yet? Is there anything I can do to move this forward?

m1-s avatar Aug 15 '22 07:08 m1-s

@HuwCampbell We are really waiting for a solution here. Any news?

m1-s avatar Aug 22 '22 08:08 m1-s

I think for now:

call it simpleVersioner and implement in a similar way to helper.

simpleVersioner :: String -> Parser (a -> a)

and skip the short option -v so we don't collide.

HuwCampbell avatar Aug 31 '22 01:08 HuwCampbell

I think for now:

call it simpleVersioner and implement in a similar way to helper.

simpleVersioner :: String -> Parser (a -> a)

and skip the short option -v so we don't collide.

Done. Let me know if anything is missing.

m1-s avatar Sep 05 '22 10:09 m1-s

Fixed build pipeline. Please run again

m1-s avatar Sep 08 '22 09:09 m1-s

@HuwCampbell

m1-s avatar Sep 13 '22 15:09 m1-s

Thanks! Merged a while back.

HuwCampbell avatar Oct 05 '22 08:10 HuwCampbell