ppx_deriving_cmdliner icon indicating copy to clipboard operation
ppx_deriving_cmdliner copied to clipboard

Positional arguments with option type crash cmdliner

Open phated opened this issue 2 years ago • 3 comments

If you specify [@pos] on a field that is an option type, cmdliner will crash with Fatal error: exception Invalid_argument("Option argument without name")

I tried digging into this but I couldn't see where the issue would be.

phated avatar May 28 '22 20:05 phated

Currently, all record fields of type option are converted to cmdliner optional arguments. It currently passes its name as None which is causing the error in the nested function calls that implement cmdliner's functionality at runtime. since optional arguments must be named. If the type of the record field is option, any other attributes not related to optional arguments (including [@pos *]) are ignored, which is not what should have happened here, of course.

I am guessing you would like something more like what Rust's structopt does with positional arguments whose underlying type is Option<T>. The issue is that I don't think this functionality is possible given cmdliners's CLI semantics. We should instead have raised an error when the PPX code generation was running explaining that positional arguments cannot be type option, which is something that we can fix pretty easily, although it may not be what you were hoping for.

What cmdliner does support, however, is the notion of a positional argument that consumes an arbitrary number of tokens (e.g., look at the files argument in the cmdliner rm example). We could conceivably add new attributes like [@pos_all] and [@pos_right *] to go with cmdliner's pos_all and pos_right which would take an arbitrary number of positional arguments and put them into a list. I realize that it is not an exact match to the functionality you wanted, but as far as I can tell, cmdliner does not support any optional positional arguments otherwise. The use of subterms for multiple/optional arguments is something cmdliner's docs explicitly warn against as likely to break in the future. We can keep this issue open to track this new feature, if you would like.

trepetti avatar Jun 04 '22 18:06 trepetti

The issue is that I don't think this functionality is possible given cmdliners's CLI semantics.

I guess I'm confused by their docs then, because it says this for pos: "If the positional argument is absent from the command line the argument is v." Which makes it seem like I could specify v as None to make it an optional value?

phated avatar Jun 04 '22 20:06 phated

In any case, even having pos_all and pos_right would be helpful, as I can't support an optional positional argument at all right now.

phated avatar Jun 04 '22 20:06 phated