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

fixed a typo in unit test, and made boolean options default to false ins... #19

Merged
merged 4 commits into from
Feb 2, 2014

Conversation

stratease
Copy link

...tead of null. added bool unit test

@stratease
Copy link
Author

couldn't find the dev branch you mention in the contribution note... hopefully that's fine.

@nategood
Copy link
Owner

Looks like the remote branch has been deleted. That's fine. Thanks!

Having said that, default support was actually just merged in a few days ago. Can you incorporate that into the pull request so we get a clean merge?

@stratease
Copy link
Author

Cool! Ok, will do.

@stratease
Copy link
Author

Made the appropriate merge. I added logic to inverse the bool default value, and of course the default is false if not specified

@@ -342,7 +342,7 @@ public function parse()

$option = $this->getOption($name);
if ($option->isBoolean()) {
$keyvals[$name] = true;
$keyvals[$name] = !$option->getDefault();// inverse of the default, as expected
Copy link
Owner

Choose a reason for hiding this comment

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

Hmm. This makes total sense, but I can imagine this potentially causing headaches if the developer isn't fully aware of this. Feels a bit awkward. Let me think on this. Totally open to hearing an argument for this case.

@stratease
Copy link
Author

Sure, np.

The way I see it in order to support bool options that are interpreted as either true or false (giving more CLI flexibility), this is essentially changing what the default initial value is, since it's always going to be the inverse of the initial "default" value.

So either we enforce this behavior using the default() API, or we'd have to modify the boolean() option to allow overriding the "default" value via some other means, i.e.

...
// some way of defining what the "default" value is...
$cmd->option('d')->boolean(false); // now our -d option is defined as false? if detected 

@stratease
Copy link
Author

Also, how would the default() API function without this inverse logic boiled into it?

// it'll always be true whether -d is passed or not?
$cmd->option('d')->boolean()->default(true); 

nategood added a commit that referenced this pull request Feb 2, 2014
fixed a typo in unit test, and made boolean options default to false ins...
@nategood nategood merged commit 93a8d97 into nategood:master Feb 2, 2014
@nategood
Copy link
Owner

nategood commented Feb 2, 2014

Thanks. That works for me. For some reason I was getting hung up on options that imply negation in their name (e.g. --no-pager), but this totally makes sense. Bumped to v0.2.5.

@stratease stratease deleted the fix-boolean branch February 3, 2014 18:00
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