c-blake / cligen

Nim library to infer/generate command-line-interfaces / option / argument parsing; Docs at
https://c-blake.github.io/cligen/
ISC License
509 stars 24 forks source link

boolean long options #117

Closed pb-cdunn closed 5 years ago

pb-cdunn commented 5 years ago
+ falconc raptor-db-calc-length-cutoff --fail-low-cov --alarms alarms.json 20000 30 --rdb reads.rdb
Bool option "fail-low-cov" non-boolean argument ("--alarms")
raptor-db-calc-length-cutoff [optional-params] 
Perform a linear pass on an overlap file, and determine rough clipping
coordinates to 'correct' reads. Write integer to stdout.
Options(opt-arg sep :|=|spc):
  -h, --help                                  print this cligen-erated help
  --help-syntax                               advanced: prepend,plurals,..
  -f, --fail-low-cov   bool    false          Exit non-zero if the input
                                              coverage was insufficient to
                                              satisfy the requirement.
  -a=, --alarms-fn=    string  ""             Optional JSON file to write SMRT
                                              Link alarms. (typically used w/
                                              -f)

It turns out that even though --fail-low-cov is a boolean option, it needs an argument, e.g. --fail-low-cov:on. The short version, -f, does not need an argument.

This isn't totally surprising. It makes sense, really. But I think it should say -f, --fail-low-cov= in the help, to indicate that the long-version requires an argument.

Very minor point, and I don't mind if you leave it the way it is. I only wanted you to be aware of it. Feel free to close this right away.

c-blake commented 5 years ago

Both short and long forms accept an optional argument, but should not need it. If you are really seeing a requirement for the value, that is a bug I don't know about somehow triggered in your situation and we should investigate more.

All these variations are what I get for the test/PerParam program (the verb: value is the thing to watch):

$ ./PerParam
alpha:1 beta:2.0 verb:false item:""
$ ./PerParam -v
alpha:1 beta:2.0 verb:true item:""
L1:~/pkg/nim/cg/test[1]$ ./PerParam --ve
alpha:1 beta:2.0 verb:true item:""
$ ./PerParam --ve:f
alpha:1 beta:2.0 verb:false item:""
$ ./PerParam --ve:t
alpha:1 beta:2.0 verb:true item:""
$ ./PerParam --ve=f
alpha:1 beta:2.0 verb:false item:""
$ ./PerParam --ve=t
alpha:1 beta:2.0 verb:true item:""
$ ./PerParam --ve-rb
alpha:1 beta:2.0 verb:true item:""
$ ./PerParam --ve-rb:
alpha:1 beta:2.0 verb:true item:""
$ ./PerParam --ve-rb=
alpha:1 beta:2.0 verb:true item:""
$ ./PerParam --ve-rb=f
alpha:1 beta:2.0 verb:false item:""
#
# short options can also accept a value
#
$ ./PerParam -v=f
alpha:1 beta:2.0 verb:false item:""
$ ./PerParam -v:f
alpha:1 beta:2.0 verb:false item:""
$ ./PerParam -v:t
alpha:1 beta:2.0 verb:true item:""
$ ./PerParam -v:
alpha:1 beta:2.0 verb:true item:""

So, my first question is: are you actually seeing the strict requirement of the argument you mention or only the optional acceptance of the argument?

As far as documentation remedies, I guess we could put the = or : in square brackets for bool. Doing that in both places adds like 4 text columns, though (6 counting the =s) for every bool, as in:

-f[=], --fail-low-cov[=]

There is a whole 6-line spiel about how bool works in --help-syntax (also invokable as just --helps, btw). I suspect we could maybe clarify that with regards to the optional nature of the argument. A PR is quite welcome to improve that --help-syntax output. It is there, but maybe too implicitly? Or maybe you didn't read --help-syntax?

pb-cdunn commented 5 years ago

I think it gets confused when another option follows the boolean long-form option.

c-blake commented 5 years ago

Seems to work for me for either short/long form following a bool, even with short form using same argv slot for option & arg.

$ ./PerParam --ve-rb -z 2
alpha:2 beta:2.0 verb:true item:""
$ ./PerParam --ve-rb -z=2
alpha:2 beta:2.0 verb:true item:""
$ ./PerParam --ve-rb --alpha 2
alpha:2 beta:2.0 verb:true item:""
$ ./PerParam --ve-rb --alpha=2
alpha:2 beta:2.0 verb:true item:""
$ ./PerParam --ve-rb=f -z 2
alpha:2 beta:2.0 verb:false item:""
$ ./PerParam --ve-rb=f -z=2
alpha:2 beta:2.0 verb:false item:""
$ ./PerParam --ve-rb=f --alpha 2
alpha:2 beta:2.0 verb:false item:""
$ ./PerParam --ve-rb=f --alpha=2
alpha:2 beta:2.0 verb:false item:""
$ ./PerParam --ve-rb=f -z2
alpha:2 beta:2.0 verb:false item:""
$ ./PerParam --ve-rb=f -z2 a
alpha:2 beta:2.0 verb:false item:""
positional[0]: 0x14d08f059d00"a"

Maybe you can give me a test case that fails? It's hard for me to do anything if I cannot reproduce the alleged misbehavior. It's ok if it's part of your falconc. Right now I have nothing to go on, really. It all just works as expected for me.

c-blake commented 5 years ago

Oh...maybe your initial report has one? That's a dispatchMulti setting? Maybe that's a salient difference?

c-blake commented 5 years ago

Hmm.

$ ./FullyAutoMulti de-mo --ve-rb --al-pha 2
alpha:2 beta:2.0 verb:true item:""

also works. I'm not sure what's going on with the argument to your --alarms option. Is that maybe a shell quoted single argv element?

c-blake commented 5 years ago

I cannot find falconc searching github for code with .nim extension. So, perhaps it's non-public in which case I'll still need your help to fix this.

pb-cdunn commented 5 years ago

It's semi-public. At the moment you can find it on the develop branch at

To run, you would need to install htslib. To install, you first run make rsync to grab the nim dependencies from the vendor/ sub-directory. (That's because we do not have access to the internet at build-time -- corporate policy.)

pb-cdunn commented 5 years ago

Ah, I just updated my subtree from your upstream master, and now it works!

(I am slowly growing more adept with "subtrees", which work-around our corporate policy of no web-access at build-time. https://medium.com/@porteneuve/mastering-git-subtrees-943d29a798ec )