mergeParams() gets called twice

[genotrance]

import cligen

proc mergeParams(cmdNames: seq[string], cmdLine = commandLineParams()): seq[string] =
  echo "Hello " & $cmdNames
  result = cmdLine

proc main(check = false) =
  echo check

dispatch(main)

On running this code, you get the following output:

Hello @[]
Hello @[""]
false

First time, mergeParams() gets called with cmdNames = @[], second time it is cmdNames = @[""].

[c-blake]

Looking into if I can streamline it. As a quick workaround, I think one could always have a mergeParams that did nothing but return if cmdNames.len == 0.

[genotrance]

Yep, that's what I'm doing for now. I just wasn't sure if it was a bug or expected behavior.

[c-blake]

Well, it's expected given the source code. ;-) It might be at least a "performance gotcha" if mergeParams does things like read config files. In the cligen/mergeCfgEnv shipped with cligen, we guard that with if cmdNames.len > 0:.

[c-blake]

I think I can probably have it called only once for ordinary dispatch which is definitely "less surprising". Look for a commit in the next day after I've tested my change more thoroughly.

This commit should not break your cmdNames.len > 0 or != 0 tests. The one remaining call will be the one with a slot-0 cmdName. But you'll be able to drop the test if you can rely on building against a recent cligen version, and future mergeParams() writers can not be surprised.

Some more details you probably don't care about: This fix might break code of anyone calling dispatchCf directly instead of dispatch, but I seriously doubt anyone does or much wants to. That dispatchCf entry point was mostly for people who do not want to use the shared clCfg global variable, but people don't really need another entry point. If they had some standard alternate config variable they import and want to use they could just import xyz; clCfg = myCfg. The parametric form just seemed "a more usual way" to override. Still, I will provide a way for those users (if any exist) to pass a seq[string] to dispatchCf. They could write a mergeParams00() or pass whatever since they're calling it themselves by assumption. Or they could switch to clCfg = myCfg style. So, both are a non-zero work, but also not-a-lot fixes for them while everyone else (which might just be everyone) gets to skip the duplicate call.

More details you might care about: For dispatchMulti, mergeParams will still be called twice (more generally N+1 times where N=1 for subcommands, 2 for sub-sub-cmds, etc.). {It's N+2 right now...} I don't think that N+1 is easy to eliminate without re-doing dispatchMulti to not use dispatchGen itself on a generated proc. The value added from dispatchGen in that context is minor, since only "leaf" commands have "real" options, not just --version|--help|--help-syntax. The subcommand dispatchers just route from their first command param to whichever dispatcher. Still, having stdopts there at every level is something. Personally, I never use more than subcommands myself. For that 1-level only case, it would be easy to change that .len test to .len==2 before doing anything much like the current situation of checking .len > 0. A cmdNames length test is no longer enough to know you've been routed to a leaf once you have variable depth like test/MultiMulti.nim.

Honestly, sub-sub and sub-sub-sub commands have been far more "in demand" than I would have anticipated. I dislike them as a CLuser, myself. More flexible support for those would also probably need a re-write away from re-using dispatchGen. That re-write could/should also handle calling mergeParams() only once in the leaf command. I do not know when I will have time to do that sort of a re-write for a use case that never comes up for me personally, though.

[c-blake]

@kaushalmodi and @pb-cdunn -- you should take a look/compile against cligen#head when you get a chance to confirm that this does not break anything. I doubt it will, but err on the side of prudence.

[pb-cdunn]

Still want me to test this? I have time now.

[genotrance]

I just put in the workaround for now and haven't had any issues.

https://github.com/nimterop/nimterop/blob/master/nimterop/toast.nim#L203

[c-blake]

As mentioned the workaround should no longer be needed for just dispatch except on old cligen versions. I know @pb-cdunn uses sub-subcommands. Some kind of extra guard may be needed there, depending. Let me know if you run into any trouble not articulated above in this thread.