-
-
Notifications
You must be signed in to change notification settings - Fork 41
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
Implement option required: true #122
base: main
Are you sure you want to change the base?
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 |
- 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
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. |
required: true
is accepted and implemented byOption
, but not used byParser
orBanner
. This PR implements the enforcement of it and surface it to the help banner.Checks: