Exit code 1 for any failed checks#125
Exit code 1 for any failed checks#125schlessera merged 10 commits intowp-cli:masterfrom flinthillsdesign:check-exit-code
Conversation
inc/class-command.php
Outdated
| $message = 1 === $check_count ? "1 check reports 'error'." : "${check_count} checks report 'error'."; | ||
| WP_CLI::error( $message ); | ||
| } else { | ||
| exit( 1 ); |
There was a problem hiding this comment.
We should use WP_CLI::halt( 1 ); so we can avoid exiting if we need to.
@wp-cli/committers What do you think about this proposed change? I don't remember my original intent in always using |
* upstream/master: Revert "Validate MIME type against extensions"
|
I think it makes sense to have a non-zero exit code on error. As an example, you could then use If the compatibility break is an issue, we should bump the major version. We're at v0.1.0, though, which would signal that the public API is not yet finalized anyway. I'm 👍 for this change. |
inc/class-command.php
Outdated
| if ( ! empty( $results_with_error ) ) { | ||
| if ( 'table' === $assoc_args['format'] ) { | ||
| $check_count = count( $results_with_error ); | ||
| $message = 1 === $check_count ? "1 check reports 'error'." : "${check_count} checks report 'error'."; |
There was a problem hiding this comment.
Can we update the tests to include coverage for this output?
There was a problem hiding this comment.
Yeah, I had some trouble with that for some reason. But I probably figured a few things out since then so I'll try again as soon as I can.
There was a problem hiding this comment.
@danielbachhuber just a heads up -- the issue was that Formatter removes $assoc_args['format'], so I had to change it do that check before formatter runs. Don't know if there's a specific reason it works that way, but it caught me off guard.
https://github.com/wp-cli/wp-cli/blob/master/php/WP_CLI/Formatter.php#L37
|
I like it too. 👍 |
|
Could somebody poke travis to run the tests? I can't come up with anything else to commit to trigger it... |
|
Probably easiest if you do it (aka I don't know how to do it) - a suggestion would be to put a space before the opening curly brace here |
|
Okay that's not working - I notice you have merge conflicts in your own branch https://github.com/flinthillsdesign/doctor-command/pull/2, maybe that's confusing Travis - maybe do a |
* upstream/master: Fix @var tag for check->_message Add value_is_not setting to option value check Move the options header for check command docs Add `--format=count` to supported formats
|
Thanks @gitlost, I closed my PR and merged upstream/master...something there got it going again. |
|
Alright, the tests are passing so I think I'm done here. Let me know if there's anything else I need to do. Thanks! |
|
Looks good to me. Requested a second review from @schlessera |
|
Merged, thanks for the PR, @davegaeddert ! |
Exit code 1 for any failed checks
5 participants
I think this should mostly make sense. The gist is that if any checks fail in a
wp doctor checkcommand, then it should use the exit code 1. As far as I could tell, it was always exiting with 0 regardless of whether the checks succeeded or not. I think this would make the command more useful, especially for automated systems/scripts.I had a little trouble testing the STDERR message, so that should probably be checked if this makes sense and can be merged.
Thanks, this is an awesome wp-cli tool!