Add Option groups #486
Add Option groups #486
Conversation
tbidne
force-pushed
the
parser-groups
branch
2 times, most recently
from
752442b
to
94f61bc
Compare
|
Decided to see how this might help out the linked diff --git a/app/Main.hs b/app/Main.hs
index cd53a8a..2a8943f 100644
--- a/app/Main.hs
+++ b/app/Main.hs
@@ -469,7 +469,7 @@ sourceTypeParser =
]
printerOptsParser :: Parser PrinterOptsPartial
-printerOptsParser = parsePrinterOptsCLI mkOption
+printerOptsParser = parserOptionGroup "Printer options" $ parsePrinterOptsCLI mkOption
where
mkOption name helpText placeholder =
option (Just <$> eitherReader parsePrinterOptType) . mconcat $Click to expand help pageCC @georgefst |
tbidne
force-pushed
the
parser-groups
branch
3 times, most recently
from
06431d5
to
37ac35e
Compare
|
Hi, thanks for talking the time to write this and justify your approach so well. You've pretty much nailed the reasons as to why this is a tricky thing for this API. The way things stand we currently have no functions apart from the core type classes which act over the Now this needn't be a strict requirement, as, for example, That said, this PR looks like a pretty reasonable solution on first read through. While the function does need to do a tree traversal, which strikes me as a little odd, it seems to be a relatively simple way to achieve the behaviour the issue is asking for. I'll have a proper look and think when I get some spare cycles. With your choice of nested cases I would probably go the other way, with inner cases taking precedence, otherwise things may not compose as well, or one could actually nest the groups as well. But that might be overkill. The other thing to note is that it will look different to command groups again, which would be a bit of an annoying inconsistency. Cheers. |
|
Hi, thanks for the quick response.
|
|
Using a nested group would just be a It's been a while since I implemented command groups, but I thought they were actually merged as well, though I would be happy to remove that logic as I think it's redundant. |
|
Edit: I think I figured out what you mean. Consecutive command groups are merged i.e. hsubparser ( commandGroup "Group 1" ... )
<|> hsubparser ( commandGroup "Group 1" ... )This will indeed be merged. What I currently have here allows for merging all (including non-consecutive) identical groups regardless of the order. It would be the equivalent of the following, for command groups: -- the two "Group 1"s will be merged
hsubparser ( commandGroup "Group 1" ... )
<|> hsubparser ( commandGroup "Group 2" ... )
<|> hsubparser ( commandGroup "Group 1" ... )It does this by sorting first, then grouping. I like it, but that's probably too opinionated, and a departure from how optparse otherwise works. Expand outdated commentHere's an example of what happens to commands: parseCommand =
hsubparser
( command "list 2" (info (pure List) $ progDesc "Lists elements")
)
<|> hsubparser
( command "list" (info (pure List) $ progDesc "Lists elements")
<> command "print" (info (pure Print) $ progDesc "Prints table")
<> commandGroup "Info commands"
)
<|> hsubparser
( command "delete" (info (pure Delete) $ progDesc "Deletes elements")
)
<|> hsubparser
( command "query" (info (pure Query) $ progDesc "Runs a query")
<> commandGroup "Query commands"
) |
This comment was marked as outdated.
This comment was marked as outdated.
|
Apologies for the moving target, but I think the conversation has possibly become hard to follow due to separate threads (my fault!), so I've pushed some of the changes to this branch, to make for an easier review. In particular, the commit changes the following:
Eventually I'd like to make my added tests more robust, so I'll probably redo those, but for now I'm happy with the way this works, so I'll wait for more feedback before changing anything. |
- Nested groups no longer overwrite; we concatenate them instead. - We no longer sort groups alphabetically; this means, in particular, that duplicate groups are only merged when they are consecutive. This behavior matches command groups.
Resolves #270.
This is an alternative to #231 by @larskuhtz, for whom I am grateful for blazing the trail.
First, thanks for the great library. This is my attempt to resolve #270, as this is a feature I really want (and it seems I am not alone) 🙂.
Also, this isn't nearly as bad as the diff numbers imply; about 80% of the changes are test boilerplate.
Background
These are notes on why solving this isn't trivial. Feel free to skip if you're already familiar with the difficulties:
Click to expand
Commands
Before I jump into the design / impl, I want to first look at why command groups work so well, and why it's harder to do the same for option groups. For commands, the relevant types are:
We can add multiple commands with
command, add a group withcommandGroup, and combine all of this together using theSemigroupinstance. It is easy to add the concept of a group here since the basic modifier --CommandFields-- already collects multiple commands together. So all we have to do is add an optional label and render it appropriately.Options
Now let's look at
Option.Unlike
CommandFields,OptionFieldsdoes not take in multiple options, so we cannot easily add an optional group label. Perhaps we can simply make one? Say,Unfortunately, this won't work! The fundamental problem is different options usually have different types -- unlike different commands, which must have the same type -- so we cannot, say, group an
Option IntandOption Stringtogether (at least not without existential types, which would probably be terrible).Solutions
As far as I can tell, there are really only two possibilities for a user-facing
groupOption :: String -> ....1. Add an optional group to
OptionFields(andArgumentFields,FlagFields)This is what the previous PR did:
The advantage is that this fits well with standard optparse usage, and it is probably the most obvious way to retrofit option groups into the existing framework.
The disadvantage is that we have to individually add
setGroup "Some group"to every option we want in"Some Group". It's clunky, and opens up the possibility for typos creating multiple groups. This is also inconsistent with how command groups work, as you only have to specify the group once for the latter. Unfortunately our hand is forced due to options having different types.I assume this is the primary reason the PR was declined, perhaps along with some concern over the necessary implementation changes.
2. Add an optional group to OptProperties
This is what this PR does. Because this is at a "higher level" than
Mod, our user-facing function is:The disadvantage is that isn't the usual
Modsemigroup pattern.The advantage is that we only have to specify the group in one place, and all "downstream" parsers will automatically belong to the same group.
The other advantage is that the code changes are quite small. Other than the rendering logic (which is nearly identical to the first PR, thanks @larskuhtz!), the only modification here is adding the new field to
OptProperties, and providing a set function.Examples
The tests show examples of what this looks like. For a "realer" example, I tried this out on a CLI app with many options. Here is the diff, and here is the original help page (brace yourself):
Click to expand
And here is the same with the new groups:
Click to expand
Remarks
There is a question of what to do in the nested case e.g.
I chose to always override the group, so in this case both
parserAandparserBwill be part ofGeneral Group. I judged this simpler, but we could choose instead to only overwrite the group when it isNothing. This would renderparserAinGeneral GroupandparserBinB group(flat; rendering is never nested).See the comment on
optPropertiesGroupin Builder.hs.Another question concerns duplicate groups e.g.
I chose to make groups unique i.e. both parserA and parserC will go in the same
Group A.See the comment on
sortGroupFstin Internal.hs.I'm not very familiar with optparse's internals, so please do let me know if I've missed anything obvious. I'm not married to this design, but I would really like to make some progress here.
Thanks!