Skip to content
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

How can I set selected subcommands to "pass through" all strings to its only seq[string] arg? #138

Closed
kaushalmodi opened this issue Apr 23, 2020 · 29 comments

Comments

@kaushalmodi
Copy link
Contributor

kaushalmodi commented Apr 23, 2020

Hello,

I have a CLI wrapper project (the same p4x I have mentioned earlier for Perforce p4 version control utility).

The wrapper handles many commands but for the ones I don't yet support in the wrapper, I want to pass the switches and args as-is to the underlying p4 executable.

e.g. p4x subcmd -a -b should pass "subcmd -a -b" to the p4 executable. The thing is that the subcmd proc I define in the wrapper does not have all the switches defined. I just want to wrapper to delegate that particular subcmd to the underlying p4.

Here's a minimal reproducible example to get the point across.

# mywrap.nim
import std/[strformat]

proc nopass(args: seq[string];
            switchA = false,
            switchB = false) =
  echo &"args = {args}"
  echo &"switchA = {switchA}"
  echo &"switchB = {switchB}"

proc pass(switchesAndArgs: seq[string]) =
  echo &"switchesAndArgs = {switchesAndArgs}"

when isMainModule:
  import cligen

  dispatchMulti(
    ["multi"],
    [nopass, short = {"switchA": 'a', "switchB": 'b'}],
    [pass]
  )

After compiling above, these work as expected:

./mywrap nopass abc -a -b
args = @["abc"]
switchA = true
switchB = true

./mywrap pass abc
switchesAndArgs = @["abc"]

./mywrap pass abc -- -a -b
switchesAndArgs = @["abc", "-a", "-b"]

But this does not. Is there a dispatch option I can enable to allow that?

./mywrap pass abc -a -b
Unknown short option: "a"

Run with --help for full usage.

Desired output:

./mywrap pass abc -a -b
switchesAndArgs = @["abc", "-a", "-b"]

It's a UX issue for my wrapper script.. I basically want it to Just Work for the user without having them to worry if that subcommand is supported by my wrapper or not.

Right now, they would remember to put that extra "--" inbetween. They might as well just call the underlying binary instead of using the binary if they need to remember to put "--" for selected subcommands.

Summary: Is there a way to make ./mywrap pass abc -a -b for the above minimal example (of course with any needed adjustments to the proc or dispatch call without needing to manually define all the args for -a, -b, etc)?

@kaushalmodi
Copy link
Contributor Author

This diff shows how I intend to use this "pass-thru" feature in the actual project.

Pass-thru "p4x unshelve" directly to regular p4

This doesn't yet work as expected because if I use a switch that p4
recognized but not the p4x proc signature, cligen complains with
something like:

    Unknown short option: "s"
    Run with --help for full usage.

if we run something like "p4x unshelve -s 12345"

Workaround is to remember to pass -- inbetween:

    p4x unshelve -- -s 12345

But if we need to remember that, the pass-thru doesn't become
seamless; the user might as well remember to just use p4 instead of
p4x for unsupported subcommands.

3 files changed, 8 insertions(+)
src/p4x.nim            | 5 +++++
src/p4xpkg/globals.nim | 1 +
src/p4xpkg/pretty.nim  | 2 ++

modified   src/p4x.nim
@@ -249,6 +249,10 @@ proc have(patterns: seq[string]) =
   else:
     p4x(p4Have, patterns)
 
+# Commands passed on to regular p4
+## unshelve
+proc unshelve(switchesAndArgs: seq[string]) = discard p4Unshelve.getP4Cmd(switchesAndArgs).execCmd()
+
 ## main
 when isMainModule:
   import cligen, terminal
@@ -301,5 +305,6 @@ when isMainModule:
     [head, usage = colorUsage],
     [annotate, usage = colorUsage, short = {"forceStdoutMode": 'v'}],
     [have, usage = colorUsage],
+    [unshelve, usage = colorUsage],
     [users, usage = colorUsage]
     )
modified   src/p4xpkg/globals.nim
@@ -52,6 +52,7 @@ type
     p4Users = "users"
     p4Annotate = "annotate"
     p4Have = "have"
+    p4Unshelve = "unshelve"
 
   P4ChangesStatus* = enum
     clsNotSet = ""
modified   src/p4xpkg/pretty.nim
@@ -144,6 +144,8 @@ proc prettyP4*(action: P4Action; jArr: JsonNode; stdoutMode: bool;
         str = &"{depotPathRaw}{m.haveRev.v}"
     of p4Diff2: # diff2 command output is never sent to prettyP4 proc.
       str = ""
+    of p4Unshelve: # These p4 commands are passed directly to p4
+      discard
     let
       strStripped = str.strip(leading = false)
     if strStripped != "": # avoid adding blank lines to the output

@c-blake
Copy link
Owner

c-blake commented Apr 23, 2020

First, you could definitely just ditch all the dispatchMulti automation and do something like this along the lines of test/SemiAutoMulti.nim.

Second, I think you can use the existing mergeParams hook to just check if the subcommand being invoked == "unshelve" and if so pre-pend a leading -- argument. The only downside might be that if the user themselves also types -- then you would get two --s which might be undesirable (I don't know if this p4 unshelve command would ignore that extra -- or what). So, you may want to also check "--" notin passed args in your custom mergeParams (or check only the [0] element or something).

@c-blake
Copy link
Owner

c-blake commented Apr 23, 2020

P.S. the default mergeParams is the trivial identity, the README has a slightly fancier one, and there is a fancier/more complete one still in cligen/mergeCfgEnv.nim. And yours would be yet another level up in that complexity tower. :-)

Also, I kind of doubt this particular need is a common-enough one to really add a new auto-feature. It really only makes sense when writing a cligen tool that wraps other tools which is probably rare. So, if/when you get the above mergeParams idea working, go ahead and close this issue.

@c-blake
Copy link
Owner

c-blake commented Apr 23, 2020

{ I have a very good median time-to-close-issues score on github. ;-) }

@kaushalmodi
Copy link
Contributor Author

And yours would be yet another level up in that complexity tower. :-)

:D

Also, I kind of doubt this particular need is a common-enough one to really add a new auto-feature.

May be it's just me. Before I started using Nim, I wrote a wrapper in bash for rg .. and was it painful.

So, if/when you get the above mergeParams idea working, go ahead and close this issue.

I'll just close it right now and open a new issue if what you suggested doesn't work for me, or if I need further help with that.

There's some learning curve in the way (looking at those tests). But I will get to them eventually because this feature request to have my wrapper pass on the unsupported commands to the lower level binary has been long overdue.

Thanks!

@c-blake
Copy link
Owner

c-blake commented Apr 23, 2020

Ok. Also, if you have half a dozen or more subcommands in your p4 thing like this you could stick them in a const passThrough= [ "unshelve", "another", ...] and in your mergeParams do an if cmdName[1] in passThrough: result = "--" & cmdLine (or whatever you want the whole overall logic to be vis a vis environment vars, config files and the like). Then you can kind of do all the problem commands at once. (Or maybe they're not problems which is why your wrapper doesn't need to simplify input to them..but problems from a cligen-perspective.)

@c-blake
Copy link
Owner

c-blake commented Apr 23, 2020

Someday there should be an FAQ for all these techniques. It would be really long, though. At least 2-3x the length of all the combined test/ programs.

Judging from questions that show up on the Nim IRC about cligen, people already don't look at those, but just "ask experts" who may be..not so expert. I hate the free-for all/unstructured chat room/who's talking to whom format, but I do grep cligen|blake on archives once in a while.

So, there is an issue of A) Too much information combined with B) This is such an off the beaten path approach combined with C) Nim itself is also off the beaten path combined with D) like 60% of new Nim users trying out CLI tools as a first project from that recent survey. All together the task of guiding the right advice/hints toward the right people at the right time seems, well, almost impossible.

@kaushalmodi
Copy link
Contributor Author

I'd suggest starting a Wiki. I'll contribute my examples to it.

But the point is to have a targeted example for each use case, as minimal as possible.

For the test cases, I understand that over time the tests kind of "feature-creep", and then they cannot serve as minimal examples for new users.

What do you think?

@c-blake
Copy link
Owner

c-blake commented Apr 23, 2020

My test programs are about 90-95% "minimal". I have from time to time tacked on a new feature wrinkle to an existing test case to keep test.sh run-time down. But they surely also do not quite "cover everything", like this mergeParams question, for example.

There are also various "properties" like the RHS of initializers being slightly different from "API Nim" (as discussed in #81) and so on that are too detailed/in the weeds for a README/even module documentation but that should maybe be mentioned somewhere besides closed issue threads.

I like the idea of a Wiki that various users can contribute to. I think I can just learn how to activate that on github, right? I mean, I see the tab to create the first page. Once I do that can anyone add pages or do they need authorization from me? Or can they even create the first page without my authorization?

@kaushalmodi
Copy link
Contributor Author

I like the idea of a Wiki that various users can contribute to.

The only negative .. they might not always work.. or some of them may regress as cligen development moves forward.

I think I can just learn how to activate that on github, right? I mean, I see the tab to create the first page.

When I click on the Wiki tab, it just redirects to the Github repo. May be you need to create the very first page.

Once I do that can anyone add pages

Yes.

@c-blake
Copy link
Owner

c-blake commented Apr 23, 2020

Well, I created a Home page. See if you can edit it.

@kaushalmodi
Copy link
Contributor Author

No. May be you need to enable the "edit by anyone" feature?

E.g. I can add/edit pages on https://github.com/nim-lang/Nim/wiki, but not on this repo.

@kaushalmodi
Copy link
Contributor Author

May be uncheck this option:

image

@kaushalmodi
Copy link
Contributor Author

If that works, it's odd that Github did not nest that option under "Wikis".

@c-blake
Copy link
Owner

c-blake commented Apr 23, 2020

I found it. I don't see a "commit changes" button. I clicked it off, though.

@kaushalmodi
Copy link
Contributor Author

OK, I can edit.. Now to come up with my "simple" example.

@c-blake
Copy link
Owner

c-blake commented Apr 23, 2020

Cool. Have at it. Successful explanation usually requires assuming a great deal about the audience which is..not so easy.

@c-blake
Copy link
Owner

c-blake commented Apr 23, 2020

Description on the other hand is often easier and not a bad place to start. :)

@kaushalmodi
Copy link
Contributor Author

Here's a quickly typed up Wiki: https://github.com/c-blake/cligen/wiki/Git-like-subcommands

You can review and fix it to your liking and as I find time, I will add more examples to match your format.

@c-blake
Copy link
Owner

c-blake commented Apr 23, 2020

Ok. I sort of feel like the README.md:"Subcommands, dispatch to object init" section covers that part ok with less strformat muss & fuss. But anything you think might help some new Nim user is fine with me. It can be hard to predict what side-cues can help some new idea to "gel". All purely voluntary/as time permits, of course, but you've seemed interested in documentation for this in the past.

@c-blake
Copy link
Owner

c-blake commented Apr 23, 2020

(and sometimes just explicit things like naming files and running nim c can help, and sometimes "lower spatial density" helps, too. I don't mean to sound overly critical)

@pb-cdunn
Copy link

(c-blake mentioned this Issue to me.)

Yes, this is relevant to my interests. But I don't really need a "passthru" since I can code that myself. I just need cligen to tell me the function name and args. If that's possible, could you point me to an example that shows how?

@c-blake
Copy link
Owner

c-blake commented Apr 27, 2020

I'm not sure what you mean, exactly. "tell me" == tell who/where/what part of the code? The wrapped proc knows its own name, unless your macro wrapper changes it from the default same-name. And the whole mechanism is to parse/pass args.

That question having been asked, I can kind of spitball and say that you could use mergeParams to pass its first parameter, the names of the main command and sub-command (and sub-sub-sub... commands...) from the "routing machinery" to some "leaf proc". E.g., you could add a --dispatchedName kind of string or seq[string] parameter (depending on your needs) to a most-nested/leaf wrapped proc. Then mergeParams could check cmdName[^1] to see if the leaf proc of interest is in play and if so prepend the used CLI with parameters that would set up dispatchName for it. No idea if this kind of construction would satisfy your request that I know I don't exactly understand.

Besides trying to "quote/cram the whole param list" and pass that as some new parameter, there is also parseOnly (see test/ParseOnly.nim). That may be hard/impossible to use directly in a dispatchMulti setting, though.

@c-blake
Copy link
Owner

c-blake commented Apr 29, 2020

@pb-cdunn - did I answer your question?

@kaushalmodi - did you get cmdline param pass through the way you wanted?

Also, @genotrance (shashlick on IRC) was asking on 2020/04/21 about what I called test/FancyRepeats.nim. That further argues for a wiki page that is a list of pointers to test programs from keywords or maybe example CL-usage. Someone should maybe add that. Not sure how to route people to the right test program/example code. I doubt there is much standard "terminology" for most of these CL features.

@kaushalmodi
Copy link
Contributor Author

@c-blake No, I still need to study how to unroll the dispatchMulti macro.

@c-blake
Copy link
Owner

c-blake commented Apr 29, 2020

Maybe I should have been more explicit in my answer. I think this works how you want (following your code example):

# mywrap.nim
import std/[strformat]

proc nopass(args: seq[string];
            switchA = false,
            switchB = false) =
  echo &"args = {args}"
  echo &"switchA = {switchA}"
  echo &"switchB = {switchB}"

proc pass(switchesAndArgs: seq[string]) =
  echo &"switchesAndArgs = {switchesAndArgs}"

when isMainModule:
  import cligen

  let passThrough = [ "pass" ] # , "pass2", "pass3", ...
  proc mergeParams*(cmdNames: seq[string],
                    cmdLine=commandLineParams()): seq[string] =
    if cmdNames.len > 1 and cmdNames[1] in passThrough:
      "--" & cmdLine
    else:
      cmdLine

  dispatchMulti(
    ["multi"],
    [nopass, short = {"switchA": 'a', "switchB": 'b'}],
    [pass])

Then usage is like:

Usage:
  j {SUBCMD}  [sub-command options & parameters]
where {SUBCMD} is one of:
  help    print comprehensive or per-cmd help
  nopass
  pass

j {-h|--help} or with no args at all prints this message.
j --help-syntax gives general cligen syntax help.
Run "j {help SUBCMD|SUBCMD --help}" to see help for just SUBCMD.
Run "j help" to get *comprehensive* help.
$ ./j nopass
args = @[]
switchA = false
switchB = false
$ ./j pass
switchesAndArgs = @[]

All I did was wedge in a custom mergeParam between import cligen and dispatchMulti as discussed. The only limitation to this approach is that you will not get much cligen-erated help for pass commands. Just the name of the subcommand, really. There is no way for cligen to even know --help works on the wrapped command.

Another approach entirely distinct from mergeParams or dispatchMulti unbundling might be a wrapper generator that converted the output of wrapped-command pass --help into Nim code. That would have no passthroughs at all, but ease the pain. The program/script doing the massaging could perhaps generate a Nim include and then all you would have to enter in the Nim might be include genwrap and then the names of generated wrappers. But I'd bet mergeParams is enough for you.

@kaushalmodi
Copy link
Contributor Author

@c-blake Ah! I thought this would a huge undertaking because I misinterpreted your response in #138 (comment) .

I looked at that First .., and then Second .. as the 2 steps I'd need to do to make this work. Turns out they were 2 alternatives, not 2 steps 😄 *phew*

@c-blake
Copy link
Owner

c-blake commented Apr 29, 2020

No problem. I followed up for a reason.

If you had many such wrapped multi-commands you could of course write a Nim template mergePass that took an array of names and spit out a proc mergeParams definition, but I suspect this to be more of a one-off. There aren't that many multi-commands out there.

Why, there might be none if RCS had not had happened to have such terse leaf-commands. CVS might then never have gone multi-cmd in the first place or just used cvs-ci or cvs/ci instead of cvs ci. The MH mail handling system did that sub-dir form e.g. mh/inc. Sadly, when POSIX standardized $PATH search they made that use-the-filesystem-for-namespacing inconvenient (cannot just have .../bin/cvs/cmds with only the .../bin in $PATH; . Zsh has setopt PATH_DIRS, though). I'm not even sure what rationale the POSIX committee had for that rule, TBH, but had cvs/ci been popular it would surely have been allowed in this alternate timeline. ;-) It's better in some ways (open architecture for aliasing via symlinks, adding new commands, etc.), but worse in others (no handy overall table of top-level help).

@kaushalmodi
Copy link
Contributor Author

Your snippet with mergeParams worked beautifully! Thanks!

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

No branches or pull requests

3 participants