Implement option required: true - #122
Conversation
|
I would suggest to remove "required" flag from Parser and Banner, rather then having required option. If option is required, that means, that it is not an option, but argument. |
|
I agree, I was also confused by having to write |
46d518b to
8ed5b2f
Compare
|
From @IvanShamatov :
Could we imagine options as keyword arguments? Sometimes it makes more sense to name the arguments rather than having nameless positional arguments. And keyword arguments used to be an "options Hash" but now can be required or not. I would leave the option to the developer to choose the style they like the best. |
|
@capripot Sure, I understand the concept of keyword arguments, I just can't remember any cli utility with that kind of required option in a wild. Do you have any examples? Anyway, I don't see why we can't have it. I will talk to the team to ask their opinion on that. |
|
@IvanShamatov Do you think we could merge this? |
|
Hi @capripot, thank you so much for your patience here. I've started looking at this, as you might notice from the few small commits I've pushed. I'll look to get it ready to merge over the coming week, and will let you know if I have any questions for you. |
|
@timriley I can refresh that PR, is that something we still would like to merge? |
0a14095 to
8502eaf
Compare
|
I also think the idea of a required option is a bit odd. Not really sure what else to call it though, since we already use "argument" in this project. Maybe just |
|
It would be really good to have this if possible. It is generally idiomatic in shell scripts to support it, not every required option is an argument - for examples you could look at any database cli like psql or mysql where options like Even if it weren't strictly idiomatic, it's certainly helpful, and given that the |
|
Thanks for the nudge on this, @LyleDavis, this is definitely going to go in. I've pushed up a few tweaks above, now I'll work on rebasing and merging. |
- Adds "# REQUIRED" to option description when required - Check is required options is passed - Refactor Usage and Banner option to reuse in short usage when erroring
Stop including the full options hash in the missing params error. Instead, name only the required options that are missing values: ``` ERROR: "my-cli" is missing required option --mandatory-option ``` When required arguments are _also_ missing, keep that existing argument error and add a line about missing required options: ``` ERROR: "my-cli" was called with no arguments Missing required option: --mandatory-option ``` Errors for commands without required options are now the same as before.
Since an option with a default always has a value, the user never needs to give it Before these were still labelled as REQUIRED in the help banner, which isn’t really true.
Build the usage line from a list of parts, printing only those that apply. This fixes a command with required options but no required arguments, which previously showed with a double space, e.g. "cmd --host=VALUE". Also put required options before the "| prog SUBCOMMAND" part. Before, they came after it, so it looked like they belonged to the subcommand. Leave alias text out of the usage line, so it stays short.
Before, the required options were added to the shared Baz fixture. Because of that, about ten unrelated specs had to pass `--mandatory-option` and expect longer options hashes. Restore the Baz fixture and its specs, and test required options in their own context with inline commands instead.
837f632 to
5400217
Compare
|
Alright, rebased and looking good. I'm going to merge this! And to answer the debate around whether this is the right move, I do think it's fair to consider options as keyword arguments. In this way I think it's reasonable to have them be required. Together, I think this also helps make for more usable CLIs, since our arguments suffer the same issue as Ruby positional arguments — there's nothing telling you at the call site what they're meant to be for. Now a CLI author can make a choice about which should go where. |
|
Thank you @capripot for the implementation, to @LyleDavis for the recent nudge, and to everyone else for participating in the discussion here! |
|
thank you very much @timriley! I will wait patiently on a release - I'm having a lot of fun rebuilding a bunch of our internal tooling on this library 😁 |
5 participants
required: trueis accepted and implemented byOption, but not used byParserorBanner. This PR implements the enforcement of it and surface it to the help banner.Checks: