Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

always populate parsed arguments with default values#97

Merged
danielbachhuber merged 1 commit into
wp-cli:masterwp-cli/php-cli-tools:masterfrom
acburdine:defaults-fixacburdine/php-cli-tools:defaults-fixCopy head branch name to clipboard
Jul 1, 2016
Merged

always populate parsed arguments with default values#97
danielbachhuber merged 1 commit into
wp-cli:masterwp-cli/php-cli-tools:masterfrom
acburdine:defaults-fixacburdine/php-cli-tools:defaults-fixCopy head branch name to clipboard

Conversation

@acburdine

Copy link
Copy Markdown
Contributor

closes #30

@danielbachhuber

Copy link
Copy Markdown
Member

@acburdine Because I'm (still) new to the codebase, can you provide an extended description of how this changes the behavior, with before and after examples and any backwards compatibility / breaking change concerns?

I appreciate you taking the time to put together this pull request. Just want to make sure I understand the full implications of what I'd be merging, and how it will impact existing users of the project.

@karoluck

karoluck commented Jun 30, 2016

Copy link
Copy Markdown

+1, as i see there is only 1 added method, which is triggered on $arguments->parse() (it is called after arguments definitions). This method fills the $argument array with default values (if these values are set in definitions). As i see, this also fixes the situation when default value is set to 0, becouse it does not pass the empty() function, but the true is that 0 is also value, so its needed to fill the $arguments array even if some arg is set to 0. These changes are backward compatible, becouse it only adds the funcionality. It can cause only fill $arguments array by default values, which is very useful.

@acburdine

Copy link
Copy Markdown
Contributor Author

@danielbachhuber ^ what he said. Apologies for not responding to this sooner, I had a response written out in the Github reply box and then I accidentally clicked on a new tab and the typed-out response disappeared 😕

@danielbachhuber danielbachhuber added this to the 0.11.2 milestone Jul 1, 2016
@danielbachhuber

Copy link
Copy Markdown
Member

Cool, let's :shipit:

I'll be tagging a release in ~3 weeks (aligned with the next release of WP-CLI), if you want to try to land any more improvements between now and then.

@danielbachhuber danielbachhuber merged commit 6f8e13b into wp-cli:master Jul 1, 2016
@danielbachhuber

Copy link
Copy Markdown
Member

@acburdine Looks like the build failed on merge (and I didn't notice the pull request wasn't built): https://travis-ci.org/wp-cli/php-cli-tools/jobs/141764499

Mind tracking the issue down?

@acburdine

Copy link
Copy Markdown
Contributor Author

Huh, I thought I'd fixed the test issues. Will take a look 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Default value for Options not returned

3 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.