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

Add Option.implies() #1724

Merged
merged 13 commits into from
May 11, 2022
Merged

Conversation

shadowspawn
Copy link
Collaborator

@shadowspawn shadowspawn commented May 8, 2022

Pull Request

Problem

See: #1358

Solution

Add support for supplying a hash of implied option values. Add implementation support to have separate implied options for positive and negative dual options.

Details:

  • a default value does not trigger implied values
  • support all option types for source option, and for implied option
  • the implied value overrides a default value, but not other option sources
  • a dual positive and negative option have separate implied options
  • not adding to help (leaving it up client to describe if desired)

Side notes:

  • switched to console for code blocks with mixed commands and output for nicer display in README
  • use sh rather than bash for code blocks which don't show output

ChangeLog

  • add Option.implies() to set other option values when the option is specified

Copy link
Collaborator

@abetomo abetomo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@shadowspawn shadowspawn marked this pull request as draft May 9, 2022 02:14
@shadowspawn shadowspawn marked this pull request as ready for review May 9, 2022 10:06
@shadowspawn
Copy link
Collaborator Author

I have added extra code to support separate treatment of positive and negative dual options based on the stored option values. (In future, this might be used to expand the .conflicts() support in similar way.)

@shadowspawn shadowspawn requested a review from abetomo May 9, 2022 10:08
@shadowspawn
Copy link
Collaborator Author

shadowspawn commented May 9, 2022

Marked for re-review after the additions. (Thanks for initial review too.)

@shadowspawn shadowspawn marked this pull request as draft May 10, 2022 06:06
@shadowspawn
Copy link
Collaborator Author

Back to draft. Improving examples, and looking into interaction with .conflicts() and .requiredOption().

@shadowspawn shadowspawn marked this pull request as ready for review May 10, 2022 06:51
@shadowspawn
Copy link
Collaborator Author

Sorted again!

Copy link
Collaborator

@abetomo abetomo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome!

@shadowspawn shadowspawn merged commit d660967 into tj:develop May 11, 2022
@shadowspawn shadowspawn deleted the feature/implies-with-bag branch May 11, 2022 06:20
@shadowspawn shadowspawn added the pending release Merged into a branch for a future release, but not released yet label May 11, 2022
@shadowspawn shadowspawn removed the pending release Merged into a branch for a future release, but not released yet label May 29, 2022
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.

2 participants