Skip to content

Use wp_parse_args() where we can - #643

Merged
scribu merged 4 commits into
wp-cli:masterfrom
johnbillion:master
Aug 14, 2013
Merged

scribu merged 4 commits into
wp-cli:masterfrom
johnbillion:master

Conversation

@johnbillion

Copy link
Copy Markdown
Contributor

A couple of methods use some code which can be replaced with wp_parse_args() for simplicity.

Comment thread php/WP_CLI/CommandWithUpgrade.php Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If we're going to make this change, let's do it right:

Rename $values to $defaults and assign the result from wp_parse_args() to $assoc_args.

@scribu

scribu commented Aug 6, 2013

Copy link
Copy Markdown
Member

Actually, since we assign the defaults to a variable, I don't see much benefit in using wp_parse_args(), since array_merge() does the job just fine.

@danielbachhuber

Copy link
Copy Markdown
Member

Actually, since we assign the defaults to a variable, I don't see much benefit in using wp_parse_args(), since array_merge() does the job just fine.

Agree, particularly because we don't need to accept a string of arguments.

@johnbillion

Copy link
Copy Markdown
Contributor Author

Agreed

@scribu

scribu commented Aug 13, 2013

Copy link
Copy Markdown
Member

@johnbillion will you update your PR or should someone else handle it?

@johnbillion

Copy link
Copy Markdown
Contributor Author

Yep I'll get it updated

@johnbillion

Copy link
Copy Markdown
Contributor Author

I made the mistake of doing these changes on the master branch. Learnt my lesson now.

scribu pushed a commit that referenced this pull request Aug 14, 2013
Use `wp_parse_args()` where we can
@scribu
scribu merged commit f3134a2 into wp-cli:master Aug 14, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants