Skip to content

Conversation

@m1-s
Copy link
Contributor

@m1-s m1-s commented Aug 5, 2022

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 m1-s force-pushed the addVersionParser branch 2 times, most recently from 40e4dfd to d759f6d Compare August 5, 2022 17:17
@m1-s
Copy link
Contributor Author

m1-s commented Aug 8, 2022

@HuwCampbell whats your opinion on this?

@tfc
Copy link

tfc commented Aug 8, 2022

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

@HuwCampbell
Copy link
Collaborator

HuwCampbell commented Aug 8, 2022

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.

@m1-s
Copy link
Contributor Author

m1-s commented Aug 8, 2022

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
Copy link
Contributor Author

m1-s commented Aug 15, 2022

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

@m1-s
Copy link
Contributor Author

m1-s commented Aug 22, 2022

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

@HuwCampbell
Copy link
Collaborator

HuwCampbell commented Aug 31, 2022

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.

@m1-s m1-s force-pushed the addVersionParser branch 3 times, most recently from b3e8e36 to 27fdb77 Compare September 5, 2022 10:37
@m1-s
Copy link
Contributor Author

m1-s commented Sep 5, 2022

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 m1-s force-pushed the addVersionParser branch 2 times, most recently from c46dc16 to 7f7adae Compare September 8, 2022 08:51
@m1-s
Copy link
Contributor Author

m1-s commented Sep 8, 2022

Fixed build pipeline. Please run again

@m1-s
Copy link
Contributor Author

m1-s commented Sep 13, 2022

@HuwCampbell

@HuwCampbell HuwCampbell merged commit 9399fd0 into pcapriotti:master Sep 13, 2022
@m1-s m1-s deleted the addVersionParser branch September 14, 2022 20:26
@HuwCampbell
Copy link
Collaborator

Thanks! Merged a while back.

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.

3 participants