c-blake / cligen

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

Add validation for bounded integral types #185

Closed SirNickolas closed 3 years ago

SirNickolas commented 3 years ago

When your wrapped function has a parameter of type Natural, or Positive, or any range[X .. Y] (probably, aliased), domain checks are bypassed when invoking it via CLI.

These types describe a precondition (an in-contract): it is guaranteed that, upon entering the function, the parameter will have a value in its range. At least, guaranteed when you perform an API call. I think, cligen should either throw a ParseError when it encounters a range violation, or disallow bounded types in the signature altogether (putting more work on a CLauthor).

A related issue: cligen could show range-restricted types precisely instead of displaying them all as int in the help message.

c-blake commented 3 years ago

I know what range types are. Have you tried to write your own argParse/argHelp for this case yet? I'm no opposed to adding something to cligen/argcvt for everybody, but I feel like it should already be possible for you to do so.

c-blake commented 3 years ago

That is looking like it's shaping up nicely except maybe it sounded like you wanted Dice to indicate its numeric range to a user..?

SirNickolas commented 3 years ago

Probably, all range types except Natural and Positive should by default be displayed as range X..Y (we can drop the underlying type for the sake of conciseness), and there should be a function, say, argTypeHelp(typedesc): string, which allows users to override type’s representation (or we can reuse the name argHelp). What would you say?

I can implement it tomorrow.

c-blake commented 3 years ago

I would say to display even more concisely as just X..Y. { Terminal width is kind of precious and the way I align things even more so and the .. already indicates a range and is likely to be about as wide as string or other common types. Most CLIs do not even give users a type column. Because CLusers may well not be as Nim-savvy as CLauthors or API users I try to push the convention that messages should be more human readable, and I do think almost all get integer/string/float/plural/singular kinds of notions. So, for example, instead of seq[string] I use plural strings. If some parameter needs more clarification then that clarifying text can go in the description column (which can at least be long/word wrap/is not as pressured for being in a table). }

I would also say that if a CLauthor does not like how this looks then they should just learn how to write their own argHelp. That's already the hook. If they are that picky then sooner rather than later they will want something like test/CustomType.nim to set all 3 columns. Maybe layering that into argTypeHelp would add tiny value, but it's pretty easy to @[] up a 3-seq and it's probably better to make the CLauthor think about the whole triple (or at least double) they'll be presenting to the user in such cases. So, I think just X..Y alone with no new layering is best.

And if you want you could say 0.. instead of Positive and 1.. instead of Natural (trading brevity for clarity...I mean we aren't going to be defaulting to 0..2^63-1 either. This is a help message not a formal specification.).

c-blake commented 3 years ago

Arguably, that first argKeys slot should never have been in the output of argHelp, but we're long past cligen version 1.0 and I doubt the clean up is worth the perturbation. I had thought it might be theoretically possible for some types to want to force long-options-only in help or something like that.

This is kind of similar to having to put parameter idents in strings all the time in parameters to dispatch. For help it lets you specify rendering, but for most other things, it is probably bad. But I think no one ever complained. Once you're at the point of using one of those params (or argHelp), the little extra complexity of quotes or an extra slot is a 2nd order thing.