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

Implement option required: true #122

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

capripot
Copy link

option :option_name, desc: "This is an option", required: true

required: true is accepted and implemented by Option, but not used by Parser or Banner. This PR implements the enforcement of it and surface it to the help banner.

  • 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

Checks:

  • Tests are passing
  • Rubocop is passing on changes contained in this PR

@capripot capripot requested a review from solnic as a code owner October 13, 2022 00:00
@IvanShamatov
Copy link
Member

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.

@capripot
Copy link
Author

capripot commented Jul 6, 2023

I agree, I was also confused by having to write option :flag, required: false but wouldn't it be a breaking change? 😕

- 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
@capripot capripot force-pushed the implement-mandatory-options branch from 46d518b to 8ed5b2f Compare July 6, 2023 18:54
@capripot
Copy link
Author

capripot commented Aug 2, 2023

From @IvanShamatov :

If option is required, that means, that it is not an option, but argument.

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.
What do you think?

@IvanShamatov
Copy link
Member

@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.
Thank you for the contribution

@capripot
Copy link
Author

capripot commented Mar 9, 2024

@IvanShamatov Do you think we could merge this? ☺️ 🙏

@timriley
Copy link
Member

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.

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

Successfully merging this pull request may close these issues.

3 participants