Compile error with 1-letter param colliding with auto|manual short option

[disruptek]

This is on v0.9.46:

 proc muse(rel_rhy = ""; rel_trg = ""; rel_jja = ""; rel_jjb = "";
            sp = ""; ml = ""; sl = ""; s = ""; lc = "") =

/home/adavidoff/git/datamuse/datamuse.nim(61, 14) template/generic instantiation of `dispatchCf` from here
/home/adavidoff/git/datamuse/deps/pkgs/cligen-#master/cligen.nim(733, 14) template/generic instantiation of `dispatchGen` from here
/home/adavidoff/git/datamuse/datamuse.nim(25, 38) Error: duplicate case label

[c-blake]

Ok. I have reproduced this on nim-0.19.2 as well as nim-devel. Thanks for the small reproduction!

It's almost surely a bug of mine, somehow. nim c -d:printDispach might offer some insight, but I will look into this myself in a couple hours.

[c-blake]

Oh...It seems related to the fact that a short option is being auto-assigned to "s" which also has its own long option. So, you could probably workaround the bug by manually setting short["sp"]='nonSchar', short["sl"]='nonSchar'.

[c-blake]

Yeah. This works:

proc muse(rel_rhy = ""; rel_trg = ""; rel_jja = ""; rel_jjb = "";
          sp = ""; ml = ""; sl = ""; s = ""; lc = "") =
   discard
import cligen
dispatch(muse, short = { "sp": '\0', "sl": '\0'})

as would short = { "": '\0' } which kills all short opts.

[c-blake]

After sleeping on it, I did realize that there is a workaround for the (Nim-general style) of using a multi-value of target for short & long option keys. Specifically, parseopt3 could (optionally) namespace-prefix short options only with something like "po3short_". Then the generated parsers could prepend said prefix to the strings they look for. This would block super-rare long param names starting with "po3short_", but total generality would be restored if CL-authors can override that namespace prefix from some default value in that probably never happen scenario. This idea does less violence to the logical structure of everything and should present lower bug risk.

I had been thinking we would have to do an as-large-as-the-number-of-params if-elif chain where we check the joint conditions (p.kind == cmdShortOption and p.key[0] == 's') or (p.kind == cmdLongOption and p.key == "s" (and similarly for all the former of branches). It's not super-high bug risk, but it is more of a change than I'd prefer just pre-1.0.

Either of these two approaches could allow the short-is-different-than-long but some other long-gets-a-short-matching-a-long scenario we get with dispatch(muse, short = {"s": 'y', "sp": 's'}) above where we want short-for-"s" to be "-y" but short for "sp" to be "-s". This is admittedly a pretty tiny corner case and just not supporting it might also be an option. (EDIT: I.e. some might say that insisting "-s" and "--s" mean the same thing is a feature, not a bug, on behalf of CL users' sanities.)

Also, I will alter the title of the issue to reflect the actual bug-that-was.

Any thoughts?

[c-blake]

In dispatchGen I could even ensure an automatically chosen prefix does not collide with any long params in play. So, that could be made something the CL author need never think about just like the if-elif approach.

The question remains if I should do it just because I can do it (either way I might). If I don't do it then removing support for the short["s"]='y' bit may be prudent so the rule is "1-char params get either no short option at all or it must be themselves", though that could be enforced by the CL author.

I mostly worry about programmer/type-directed dispatch-rather-than-end-user-personalities arguing back that they want full generality more for its own sake than practical use. It is conceivably useful since short options can be bundled into one cmd arg but long options cannot. So, a 1-char param may not be a bool but you would like to use its char for an actual bool for the bundlability in spite of - vs -- completely altering meaning.

I feel like I'm mostly talking myself into the "need" for the full generality, but if you have a contrary opinion it would be good to voice it soon.

I'm honestly a little surprised this issue was only discovered now.

[disruptek]

The need is more about soft issues of programmer comfort than it is about technical need, in my opinion. So I'm :+1: for it.

[c-blake]

Ok. I agree. It's also just easier to say "cligen does whatever short tells it to" than try to describe special non-collision rules. Thanks for the pondering.

The current tip/head should be ok for your current situation. It probably isn't too much effort to get the fully general case working which I should do before 1.0. You should get notified since I'll reference this discussion in the commit.

[disruptek]

Thank you, sir! :grin:

[c-blake]

So, I started working on that and there were just so many contacts with short and long options all over the place that I decided to reverse my previous position/tilt. I think a better error message about the actual problem is good enough for >99% of use cases. Let me know if you think the error message is not good enough.

My resistance is mostly the large amount of kind of tedious work to make sure all the different points of contact work right. So, if you (or someone) wants to do a PR, I'm not dead set against the idea, but it seems too close to almost negative yield to justify right now. Maybe post-1.0 we can relax this very weak constraint.