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 requires #31

Merged
merged 7 commits into from
May 9, 2014
Merged

Add requires #31

merged 7 commits into from
May 9, 2014

Conversation

enygma
Copy link

@enygma enygma commented May 1, 2014

Adding the concept of "required options". Adds dependency between options such that:

$cmd = new Command();
$cmd->option('test')
->requires('other');

So that if "--test" is defined, "--other" must be also otherwise an exception is thrown. Options can also have multiple dependencies:

$cmd = new Command();
$cmd->option('test')
->requires(array('other','option'));

Where both "--other" and "--option" must be defined.
Unit tests are included.

@mackenza
Copy link
Contributor

mackenza commented May 1, 2014

I think this is an interesting addition but I wonder about calling it "requires" since there is already a command method by that name.

How about "dependsOn" or something like that?

@enygma
Copy link
Author

enygma commented May 1, 2014

I'm good with that...happy to make that change.

@mackenza
Copy link
Contributor

mackenza commented May 1, 2014

note that I am just a user of commando not directly associated with the project ;) @nategood needs to be the one to weigh in on the PR.

I was looking further at this and had a thought... yes, dependsOn() is better than requires() as it would surely get confused with require(), but what if we just overload require()? for example:

// option 'test' is required for $cmd
$cmd = new Command();
$cmd->option('test')
->require(true, NULL);
// option 'test' is required for $cmd and option 'foo' is required with option 'test'
$cmd = new Command();
$cmd->option('test')
->require(true,'foo');
// option 'test' is optional for $cmd but option 'foo' is required *if* 'test' is present
$cmd = new Command();
$cmd->option('test')
->require(false,'foo');
// option 'test' is required for $cmd and options 'foo' + 'bar' are also required with option 'test'
$cmd = new Command();
$cmd->option('test')
->require(true,array('foo','bar'));
// option 'test' is optional for $cmd but options 'foo' + 'bar' are required *if* option 'test' is present
$cmd = new Command();
$cmd->option('test')
->require(true,array('foo','bar'));

Maybe this is too complex but I like it as many different dependency use cases can be covered by a single option.

@enygma
Copy link
Author

enygma commented May 1, 2014

It's an interesting idea, but I wonder if it's missing too many "require" contexts into on place since it'd be both: "this option is required" and "If you use this, you're required to use these others". I don't know if the (subtle?) distinction between them is enough to warrant them being different though.

@mackenza
Copy link
Contributor

mackenza commented May 1, 2014

I guess the way I was thinking about it was: The require() specifies all the dependencies of this option() or flag()... including the dependency on itself.

But you are right, it could be too subtle. Just a thought.

@enygma
Copy link
Author

enygma commented May 2, 2014

It would't take much to use what I've already put in there and overload the requires, really...it could be done either way and have the same effect.

@nategood
Copy link
Owner

nategood commented May 5, 2014

Thanks for adding this! Sorry for not chiming in earlier. I generally tend to lean towards explicitness and readability. I think @enygma also has a point w.r.t. coupling general require behavior with dependency behavior. For these reasons, I like the idea of a new method.

Agree requires is too close to require. I'm not sure dependsOn is the most descriptive name IMO. How about needs?

$cmd = new Command();
$cmd->option('test')
  ->require()
  ->needs('foo','bar');

@enygma
Copy link
Author

enygma commented May 5, 2014

Works for me on the "needs"....I wonder about that syntax though. Personally, I think if there's more than one defined it should be given in an array to avoid the need for something like func_get_args to work with a variable number of parameters.

@nategood
Copy link
Owner

nategood commented May 5, 2014

Agree. The array syntax would be more appropriate.

@enygma
Copy link
Author

enygma commented May 6, 2014

Give that a shot...

nategood added a commit that referenced this pull request May 9, 2014
@nategood nategood merged commit 6581935 into nategood:master May 9, 2014
@nategood
Copy link
Owner

nategood commented May 9, 2014

Great! Merged. Will update docs, version, etc. shortly.

@enygma
Copy link
Author

enygma commented May 9, 2014

Nice! Thanks for including this in the project :)

aphazel pushed a commit to aphazel/commando that referenced this pull request Sep 8, 2014
* upstream/master: (33 commits)
  Add getOptions method for getting all options
  Update README.md
  add docs for needs
  version bump
  update the example to demostrate longer alias working
  Moving from "dependsOn" to "needs" on Option (Issue nategood#31)
  moving "dependsOn" over to "needs" as per Issue nategood#31
  updating from "requires" to "dependsOn" for dependency handling
  adding tests for new "requires" handling in options
  Adding tests for new "required" handling to Command class
  Adding "requires" handling to Option for defining other options that must be set
  Adding functionality to Command for setting requirements on options
  Typho '-' on error message
  allow hyphen in option names
  Add a Bitdeli badge to README
  Version Bump 0.2.5
  oops - used 5.4 syntax for arrays
  integrated my bool default to false logic to the new default feature, and made the inverse of the default specified when the value is determined
  fixed a typo in unit test, and made boolean options default to false instead of null. added bool unit test
  add support for default values for unspecified options
  ...
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