c-blake / cligen

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

surprise: setByParse causes help to be omitted #125

Closed disruptek closed 4 years ago

disruptek commented 4 years ago

I see that this seems to be intentional according to the code, but it was a surprise to me. I wonder what other behavior is changing as a result of this parameter? Is there a less invasive way for me to figure out parsed parameter order?

c-blake commented 4 years ago

First, I doubt there is any other way to get user-issued parameter order (except maybe doing your own parseopt3 loop just external to cligen logic, but this would probably not interact well with/honor mergeParams).

Second, this is kind of an "advanced mode" I did for those insistent on using cligen to write CLIs that have "greater capability" than a regular Nim proc import/call style API would.

As a consequence of the latter, this mode is by its nature less automatic than the for-dummies all-in-one modes of cligen. I think you may be the very first person to use this feature besides my test/ParseOnly.nim. So, I'm not surprised if it's a little surprising and/or buggy.

I imagined it being used is some variation on test/ParseOnly.nim. So, you should definitely read that carefully. I didn't intend for setByParse to imply parseOnly=true if that's what you mean by help being omitted. Maybe it does by accident?

A "sledgehammer workaround" would be to call dispatchGen twice - once with a setByParse (and probably a dispatchName=fooParseOnly or something) and once "the normal way". Then you could do a first run with a parseOnly to set up the seq[ClParse] with the first dispatch. The next run with an ordinary dispatcher should loop over all the same data in the same order and with neither setByParse or parseOnly be maximally "ordinary", but the seq[ClParse] should be set correctly. If you don't expect your argument lists to be super long the extra expense is likely negligible.

Some pull request to fix/adapt is welcome. I'm mostly brainstorming an alternative because honestly I'm not sure what the problem is..maybe your next issue? It's hard for me to help not knowing the precise problem.

disruptek commented 4 years ago

Well, it might be just a documentation issue. When I setByParse and use --help, I get clHelpOnly as one of the parse results with the message field holding the help. It's easy to work with -- just output it. Ditto any other parse results which aren't clOk.

The problem is, I created my app with setByParse and I hadn't gotten around to actually examining the parse results by the time I first ran it. So, inexplicably, I had no output during any parse failure or help request.

Now that I know what it's doing, it's working great for me -- exactly what I needed. :wink:

FWIW, this is the app and as it doesn't actually do anything, it's almost entirely option-oriented and should be easy to follow: https://github.com/disruptek/gully

Depending on the order of arguments, gully will prioritize the user's demands over the defaults. It's just a way to simply rank the options in order of importance; the ranking is backfilled with unspecified options. Trivial to implement thanks to cligen! :smile:

disruptek commented 4 years ago

This is where I'm exploiting setByParse: https://github.com/disruptek/gully/blob/master/src/gully.nim#L516

c-blake commented 4 years ago

I will look into this in more detail in a few hours.

c-blake commented 4 years ago

I see the problem now. It's kind of a pervasive issue. In setByParse mode, cligen parses as much as possible in spite of errors (except for early help/version exits) while in regular mode it will raise exceptions. And for early help exit I just tack on a ClHelp rather than printing it. I think I was only really thinking of parseOnly=true mode when adding that setByParse stuff. Probably there need to be a bunch of if not parseOnly: clauses near setByP[] assigments to print things/raise exceptions.

c-blake commented 4 years ago

Should work as expected now. Not too hard to fix. parseOnly and setByParse should now be independent features.

disruptek commented 4 years ago

Seems to work perfectly, thanks! I'm sure it'll save the next person some head-scratching. :smile: