-
Notifications
You must be signed in to change notification settings - Fork 116
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
Option modifier for grouping options in help message #231
Conversation
4707a54
to
07db6f7
Compare
Hi,
Thoughts? Cheers, |
Hi Huw, To your first point: I am happy to add tests if this PR has a chance be be accepted. To your second point: In the past I used CmdArgs which supports grouping of options in the help message, but I didn't like the impure evaluation magic of its approach. So, when optparse-applicative was released, I started using it instead but I always missed this feature. Unfortunately, I also never could find a good work around by implementing it on top of optparse-applicatives public API. Therefor I looked for a simple way to implement grouping and this PR is the easiest solution I came up with. This doesn't mean that there aren't better ways of doing it. More concrete, I think the approach to add the group-description to each option in the group has pros and cons:
I don't really know how important the last point is. For my use cases I would probably be fine to group options by labeling a compound parser, but I haven't explored how that could be implemented. |
Options/Applicative/Types.hs
Outdated
| ArgReader (CReader a) -- ^ argument reader | ||
= OptReader (Maybe String) [OptName] (CReader a) ParseError -- ^ option reader | ||
| FlagReader (Maybe String) [OptName] !a -- ^ flag reader | ||
| ArgReader (Maybe String) (CReader a) -- ^ argument reader | ||
| CmdReader (Maybe String) | ||
[String] (String -> Maybe (ParserInfo a)) -- ^ command reader |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having a (Maybe String)
on every constructor seems a bit rough. It should probably be moved to the Option
type.
instance HasGroup OptionFields where | ||
setGroup n fields = fields { optGroup = Just n } | ||
getGroup = optGroup | ||
instance HasGroup CommandFields where |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having an instance for CommandGroup
will almost certainly cause confusion. If I use the same name for a command and an option do they appear in the same place?
They should either be separate concepts or the same, but not both.
I don't think this is the right API for this at the moment. I'm going to close this PR, but I do hope we can continue to think about this in the future. |
Thanks for contributing to optparse |
This PR introduces a
group
modifier for options, arguments, and flags.Options, arguments, and flags with a matching group modifier are listed in a section in the help message as shown in the following example:
Implementation
The implementation is similar to the recently introduced
commandGroup
field modifier for subparser commands.This PR makes the following API changes:
Options.Applicative.Types
: add new field to the theOptReader
,ArgReader
, andFlagReader
constructors ofOptReader
.Options/Applicative/Builder/Internal.hs
: add new record fields toOptionFields
,FlagFields
, andArgumentFields
.While it clearly is an API change, it's impact is limited because the affected types are exported only through the modules where they are defined and are not re-exported in the top-level
Options.Applicative
module or one of the modules directly re-exported byOption.Applicative
. There are common patterns to use record types in a way that is robust under addition of new record fields.