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. |
|
So I'll probably merge this in the future; I just need to find some time to do a bit of tyre kicking and make amendments. |
|
Great, thanks! I will likely get to the test updates I mentioned this weekend. Please let me know if there is anything I can do to help. |
tbidne
force-pushed
the
parser-groups
branch
3 times, most recently
from
30a5b05
to
cef907d
Compare
|
I made the test changes I mentioned earlier. In particular, I added a test that shows the new parser groups have the same duplicate semantics as command groups. I also squashed the commits together. With that, I am satisfied until there is feedback. Some notes for review:
|
|
Thanks for removing those imports @HuwCampbell (leftovers from a previous patch). Your CI is running into a problem with haskell-ci. See here for a workaround and links to more discussion (I am happy to make the same change here, if you'd like). |
|
Yeah I noticed... thanks for the link, should save me a bit. |
|
Yeah, put up a separate PR for it if you like and rebase on to it. |
|
I've done it. |
Adds the ability to group options together, similar to command groups. The parser groups semantics are: 1. Consecutive duplicate groups are merged. 2. Non-consecutive duplicate groups are __not__ merged. 3. Nested groups are concatenated together e.g. if "Group B" is parsed within "Group A", then B's options will render under "Group A.Group B". 1 and 2 is the same behavior as Command groups.
HuwCampbell
force-pushed
the
parser-groups
branch
from
ca04bf2
to
8a41abe
Compare
|
Ah I forgot to add the new test to |
The (<<+>>) operator was swapped for (<</>>) in c1bf93b to fix pcapriotti#360. This was mistakenly reverted when implementing the new rendering logic for option groups. This commit re-establishes the fix.
|
Fixed CI (passed on my repo). |
|
Ta. I'm patching up a few small things, just a bit time poor. I think nesting instead of dot separating would look nicer - it would be less repetitive and feel like scoping is respected. |
|
I don't disagree that scoping is more natural, but do you have an idea of what this should look like? My thoughts were something like this (notice My implementation is quite naive. It replaces the option groups' diff --git a/src/Options/Applicative/Builder.hs b/src/Options/Applicative/Builder.hs
index 0f78281..7ccecf7 100644
--- a/src/Options/Applicative/Builder.hs
+++ b/src/Options/Applicative/Builder.hs
@@ -390,9 +390,11 @@ option r m = mkParser d g rdr
--
-- will render as "Group Outer.Group Inner".
optPropertiesGroup :: String -> OptProperties -> OptProperties
-optPropertiesGroup g o = o { propGroup = OptGroup (g : gs) }
+optPropertiesGroup g o = o { propGroup = Just newGroup }
where
- OptGroup gs = propGroup o
+ newGroup = case propGroup o of
+ Nothing -> OptGroup 0 g
+ Just (OptGroup i innerGroup) -> OptGroup (i + 1) innerGroup
-- | Prepends a group per 'optPropertiesGroup'.
optionGroup :: String -> Option a -> Option a
diff --git a/src/Options/Applicative/Types.hs b/src/Options/Applicative/Types.hs
index 9693db5..2619824 100644
--- a/src/Options/Applicative/Types.hs
+++ b/src/Options/Applicative/Types.hs
@@ -151,16 +151,9 @@ data OptVisibility
-- | Groups for optionals. Can be multiple in the case of nested groups.
--
-- @since 0.19.0.0
-newtype OptGroup = OptGroup [String]
+data OptGroup = OptGroup Int String
deriving (Eq, Show)
-instance Semigroup OptGroup where
- OptGroup xs <> OptGroup ys = OptGroup (xs ++ ys)
-
-instance Monoid OptGroup where
- mempty = OptGroup []
- mappend = (<>)
-
-- | Specification for an individual parser option.
data OptProperties = OptProperties
{ propVisibility :: OptVisibility -- ^ whether this flag is shown in the brief description
@@ -169,7 +162,7 @@ data OptProperties = OptProperties
, propShowDefault :: Maybe String -- ^ what to show in the help text as the default
, propShowGlobal :: Bool -- ^ whether the option is presented in global options text
, propDescMod :: Maybe ( Doc -> Doc ) -- ^ a function to run over the brief description
- , propGroup :: OptGroup
+ , propGroup :: Maybe OptGroup
-- ^ optional (nested) group
--
-- @since 0.19.0.0A consequence of this is that the help text is no longer globally aligned. Probably it is possible to have the same alignment, but this is what I was referencing earlier when I thought the rendering logic might be tricky. At least it was not clear to me what the output should look like, nor necessarily how to achieve it. But perhaps I'm missing something obvious. |
|
Rule of thumb is to be as polite as possible, so some regrouping to make things For you program I would do this: The "algorithm" is relatively simple:
I don't think this sort of nesting will happen too much in the wild, but we should make it nice to look at. |
|
I do in fact like the consolidation, but note that this is different from how command groups operate. I.e. command groups are only combined when they are consecutive. For those that are split, they are rendered separately. E.g. the following (note that parseCommand =
hsubparser
( command "list" (info (pure List) $ progDesc "Lists elements")
<> commandGroup "Info commands"
)
<|> hsubparser
( command "delete" (info (pure Delete) $ progDesc "Deletes elements")
<> commandGroup "Update commands"
)
<|> hsubparser
( command "insert" (info (pure Insert) $ progDesc "Inserts elements")
<> commandGroup "Update commands"
)
<|> hsubparser
( command "query" (info (pure Query) $ progDesc "Runs a query")
)
<|> hsubparser
( command "print" (info (pure Print) $ progDesc "Prints table")
<> commandGroup "Info commands"
)is rendered as: |
|
I'm ok with improving the rendering of command groups. |
Fixup help text. Move worker function to where it's used. Remove Applicative do from test suite. Small tidies.
src/Options/Applicative/Internal.hs
Outdated
| fst' (_, (x, _)) = x | ||
|
|
||
| addIdx :: [(a, b)] -> [(Int, (a, b))] | ||
| addIdx = zip [1 ..] |
There was a problem hiding this comment.
Asymptotics look good, nice job.
|
This is coming together very well, I'm happy. I'll spend a bit more time reviewing over the weekend. Is there anything else you would like to add or comment on before I merge? |
|
Thanks for the quick review! The docs may need some updating in light of the change in representation. I had also planned to clean up the commit history a bit, but I can leave it as-is if you'd prefer to keep it or change it yourself. With that out of the way, there is one weird edge case. Consider something like: parserOptionGroup "Group A" $ parserOptionGroup "Group B" ...i.e. But in fact we will have That is, we will increase On the one hand, it would be nice for this to "do the right thing". On the other hand, it seems pretty pathological to me (who would want this?), and tbh, I'm not sure how we'd do the rendering correctly even if we wanted to. Suppose we always preserve all group names i.e. we instead have
But how would we know? The only thing I can think of is using something like Tbh it's hard for me to care too much about this due to the obscurity and difficulty. But if you'd like to try to address this and have ideas, I'm open to suggestions. Edit:I think I can solve this issue using the crude idea above i.e. keep track of the printed groups in a list. It's ugly and O(n^2) (because it uses |
It is possible that someone defines a "parent group" that only contains child group(s) i.e. it does not contain any top-level options. This is a problem, because the parent name will not be attached to any options, thus will not be printed. To fix this, we instead represent the Option Groups as a hierarchy list of group titles e.g. ["Parent 1", "Parent 2", "Child group"]. Then, when rendering, we keep track of which group titles have already been printed. If some group has parent titles that have not yet been printed, we prepend their titles to the normal output.
|
Just pushed up a fix to the issue I described (and also updated some documentation that needed it anyway). Please let me know what you think. Edit 2: Actually, I retract Edit 1. The current branch appears to handle this just fine. I don't love the nested logic, as I think it's a bit tricky. But it handles all the weird, pathological cases I've thrown at it so far. Edit 1: Thought of a pathological example that this currently does not handle 🙃: Duplicate nested group. If you write |
|
I've merged and will go for a release this month. Thanks for contributing and great work. |
|
Thanks for merging and all your help! |
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.
Currently, nested groups are concatenated i.e.
General group.B group.Click to expand outdated description
I chose to always override the group, so in this case both `parserA` and `parserB` will be part of `General Group`. I judged this simpler, but we could choose instead to only overwrite the group when it is `Nothing`. This would render `parserA` in `General Group` and `parserB` in `B group` (flat; rendering is never nested).See the comment on
optPropertiesGroupin Builder.hs.Another question concerns duplicate groups e.g.
We treat these the same as command groups, that is, duplicate groups are only merged when they are consecutive. The above example will render three groups,
Group A,Group B,Group A.Click to expand outdated description
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!