Nested Commands #24

[rgrinberg]

This is my stab at #24. It seems to be feature complete with arbitrary command nesting, see groups.t for a demonstration.

I'm thinking of dropping the default choice at the top level of nesting for consistency with subcommands. That would match dune's behavior of $ dune summoning the main help page.

[dbuenzli]

Thanks for your work @rgrinberg. I don't think I'll be able to have a look before the beginning of next year.

I'm thinking of dropping the default choice at the top level of nesting for consistency with subcommands.

You mean showing the help ? I was thinking a bit about the default behaviour on tool group I think it's better to error with a missing command name and the alternatives rather than show the help.

In particular for when you use tool in scripts via e.g. tool group $(SUB) it's preferable to get a non-zero exit code and an error message than a zero exit code and a man page in the face.

[dbuenzli]

I had a quick look this morning, it's nice that it doesn't look too invasive. If I understand correctly you do not do anything special regarding docs except for listing COMMANDS like Term.eval_choice did on the main term – that seems reasonable.

One thing that nagged me while looking at this is: can we have Term.eval_choice piggy back on the new groups (maybe with an ad-hoc internal flag to account for differences if there's any) ?

Besides, I know Cmdliner's API became an incredible mess (I'm often thinking about offering a more streamlined API under a new toplevel name) but a bit of documentation wouldn't hurt. Could you at least provide a first shot at doc strings in the .mlis please.

Also I'm quite convinced that a group without the subcommand should error, not show the help.

[rgrinberg]

One thing that nagged me while looking at this is: can we have Term.eval_choice piggy back on the new groups (maybe with an ad-hoc internal flag to account for differences if there's any) ?

Let me give that a try.

Could you at least provide a first shot at doc strings in the .mlis please.

Will do. Usually I do this at the end of the code review, to avoid intermediate changes to the docs. But it sounds like you agree with the API.

Also I'm quite convinced that a group without the subcommand should error, not show the help.

Sounds good to me. It will require us to change $ dune to error instead of displaying help, but that's better behavior as you explained. @jeremiedimino do you agree?

[ghost]

What would the error look like?

[dbuenzli]

It will require us to change $ dune to error instead of displaying help,

I don't think that's needed, at the toplevel we need to keep the ability to have a default command. That default command could spit out the help.

[rgrinberg]

I added documentation to the Group module and implemented eval_choice on top of it. Omitting a command results in an error.

Seems to be working well.

[dbuenzli]

Omitting a command results in an error.

Sorry I did not look at the details yet but I'm not sure I got that comment. Can we still get a default command if the toplevel sub-command is omitted ? Breaking this is a no-go.

[rgrinberg]

Can we still get a default command if the toplevel sub-command is omitted ?

Yes, this behaviour is preserved. Only the omission of subcommands at lower levels results in an error.

The re-implementation of eval_choice in this PR should be indistinguishable from the old one. Any differences are unintentional oversights.

[dbuenzli]

Thanks ! This looks very good then.

As I said it may take me a bit of time to merge (ping if it gets too long). But if further tweaks are needed I'll likely make them myself.

Just one last thing, I noticed some strange stuttering in darcs.t with TERM=dumb but that may be a bug that was there before.

[rgrinberg]

As I said it may take me a bit of time to merge (ping if it gets too long). But if further tweaks are needed I'll likely make them myself.

Np. We're in no hurry on the dune side.

Just one last thing, I noticed some strange stuttering in darcs.t with TERM=dumb but that may be a bug that was there before.

Yes, I was meaning to ask about that. This issue is independent of my PR.

[rgrinberg]

Gentle ping @dbuenzli

[dbuenzli]

This work is not being ignored :-)

[shonfeder]

Could this be configurable, so that the user can supply a default behavior if no subcommand is given? We'd need this for the behavior planned here: ocaml/dune#4367 (comment)

[dbuenzli]

IIRC. I'd rather not. See the discussion starting here.

[shonfeder]

It seems common to have a natural default behavior for sub commands (see git branch or git remote or opam switch etc.) (as you pointed out on linked conversation, in which, it seems, you were undecided at the time). It's also worth noting that @jeremiedimino, who had suggested the limitation, has actually requested precisely this behavior in the linked comment!

I wonder if the seeming recurrence of this pattern might cause a reconsideration on this point? :)

[dbuenzli]

I'm actually reconsidering.

I think in the general case selecting a nested command named cmd : string list should work as follows. Given command line arguments args : string list without the exec name:

1. Let sel : string list be the list of all arguments of args not prefixed by a - and occuring before a potential --.
2. If the cmd is a prefix of sel (and there's no longer command matching sel) then cmd is the nested command to use. cmd is then removed from args and the result is parsed using cmd's term.

This allows to have a default command at any level (if that command wants positional arguments they have to be specified after a --). Besides in contrast to the current behaviour for multi commands which requires the first argument of args to be the name of the sub command. It allows to specify options before it, as long as those do not use the non-glued forms (like -o file or --output file).

The end user can still end up being confused in many ways while refining cli invocations, but that's a bit in the dna of this poor interface medium and the behaviour seems not too hard to explain and understand.

[dbuenzli]

Of course that doesn't work :-) (e.g. js_of_ocaml -o bla.js breaks, it tries to parse bla.js as a command).

Even though I find that annoying, I think we are forced to require that no optional arguments occur before subcommand specifications (as was always the case in cmdliner).

[dbuenzli]

I'm thinking out loud and I'm not asking you @rgrinberg to look into what I'm saying, though if you wish you are welcome. But I tried to use the PR today for one subcommand in a project of mine already using eval_choice.

I stopped because the change was too invasive as it required to change all other commands aswell to use the new grouping mecanism in order to be able to use Term.Group.eval in my main function. This is not a good sign.

I think the existence of all these eval_ function are the sign that the choice problem is solved at the wrong level. The choice mecanism needs to live in 'a Term.t. In fact eventually there should be a single eval function (and eval_choices kept for backward compat.) and choice should be a combinator:

choose : ('a t * info) -> 'a t

This means that the type for Cmdliner_term.t terms which is currently simply:

Cmdliner_info.args * 'a parser

should likely be extended to statically gather the tree of choices. This is all a bit hand-wavy but I have the impression that this is the way to go. Personally I hope I can get back to that problem at some point this summer. But I won't merge this PR before at least trying to explore that direction, if @rgrinberg or someone else wants to explore it before I manage to, most welcome.

[rgrinberg]

Daniel Bünzli ***@***.***> writes:

I think the existence of all these `eval_` function are the sign that the choice problem is solved at the wrong level. The choice mecanism needs to live in `'a Term.t`. In fact eventually there should be a single `eval` function (and `eval_choices` kept for backward compat.) and choice should be a combinator:

It's what I tried initially actually. I gave up with the impression at that it would require a rewrite of a large chunk of cmdliner internals to make it work this way. The approach that I took in this PR was much less invasive in comparison.

should likely be extended to statically gather the tree of choices. This is all a bit hand-wavy but I have the impression that this is the way to go. Personally I hope I can get back to that problem at some point this summer. But I won't merge this PR before at least trying to explore that direction, if @rgrinberg or someone else wants to explore it before I manage to, most welcome.

I probably won't have time either, but I agree that it's the way to go if at all possible. Though personally, I did not find it very difficult to refactor dune's cli to use the group based api in this PR. Even if it is a little clunky.

[dbuenzli]

On 11 May 2021 at 20:46:44, Rudi Grinberg ***@***.***) wrote: It's what I tried initially actually.

Then we are on the right track!

I probably won't have time either, but I agree that it's the way to go if at all possible. Though personally, I did not find it very difficult to refactor dune's cli to use the group based api in this PR. Even if it is a little clunky.

As discussed elsewhere I'm already not really happy with the current API so I'd rather not make it worse. Especially since we have the opportunity to add a feature while actually simplifying the API (eval_choice is no longer needed). D

[dbuenzli]

Something is now available on master. See the comment here for further details.

Thanks @rgrinberg for your work. Despite that I'm not merging this it has not been unuseful and helped to get to the current design.

[rgrinberg]

Thanks for your hard work. I'll try master with dune and report how it works. Rudi.

…

On Jan 18, 2022, 2:02 PM -0700, Daniel Bünzli ***@***.***>, wrote: Closed #123. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: ***@***.***>