Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Positional arguments with option type crash cmdliner #51

Open
phated opened this issue May 28, 2022 · 3 comments
Open

Positional arguments with option type crash cmdliner #51

phated opened this issue May 28, 2022 · 3 comments

Comments

@phated
Copy link

phated commented May 28, 2022

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.

@trepetti
Copy link
Collaborator

trepetti commented Jun 4, 2022

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.

@phated
Copy link
Author

phated commented Jun 4, 2022

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
Copy link
Author

phated commented Jun 4, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants