-
Notifications
You must be signed in to change notification settings - Fork 24
Make use of report_batch_operation_results() for install commands #63
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
Make use of report_batch_operation_results() for install commands #63
Conversation
|
|
||
| if ( in_array( $language_code, $available, true ) ) { | ||
| \WP_CLI::warning( "Language '{$language_code}' already installed." ); | ||
| \WP_CLI::log( "Language '{$language_code}' already installed." ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a skip or a success? Looks like media would treat it as a success.
https://github.com/wp-cli/media-command/blob/337459d/src/Media_Command.php#L590-L600
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the success message, I think we can leave it as is.
Success: Installed 0 of 1 languages (1 skipped).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, makes sense to me.
|
@schlessera Is there anything stopping this from being merged? |
|
We may want to consider using a table for the output here too, see #64 (comment). |
|
I think we can optimize the output in follow-up PRs. |
Make use of report_batch_operation_results() for install commands
Fixes #61.
Example output: