Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add wp-cli integration #458
Add wp-cli integration #458
Changes from 6 commits
4aa93bf
4b4b75c
9508699
a9d4944
b28cd1b
b39c7bc
4bdcdd9
745e077
2218fdc
75c2f9c
37542b3
e998dc0
097bfc8
8d61a59
92a3b76
98b127b
d82740e
6879e1f
8660260
d60d1e3
4131d01
5b520e1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Note that output of
log()
is discarded to STDOUT when--quiet
flag is passed. But here I think we should display JSON output whatever case if quiet flag is present or not.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.
Ok, I used
line()
to log the results.I found 2 issues regarding wp-cli loggers while working on this:
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.
Not sure I agree with bypassing
--quiet
, I would expect no output at all. An alternative is to make sure that the program always exits with a descriptive code; ie. 0 for OK, non-0 for errors.edit: On reading @matiasbenedetto's comment below, seems like the checks are not meant to return a boolean.
Still, I don't think bypassing
--quiet
is cool.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.
I think this was fixed on the latest commits.
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.
Execution stops if a
WP_CLI::error
is thrown, so the errors are not being displayed. Could this line be moved above the $success check?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.
I'm not sure, i'm seeing this logic here:
Could you explain why you think we should move it?
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.
I don't think this is correct.
$success
is the result ofrun_themechecks_against_theme
which returnsfalse
if the theme fails a check.Using
twentytwentyfive
, here is the output currently:Moving call to
display_themechecks_in_cli
above the conditional, the result is:I think the latter is more useful because it reports what checks failed (the first one marked
REQUIRED
).Also I am not sure we should throw an error here if
$success
isn't true, because the theme check successfully ran, but the result was it failed the checks.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.
This command's focus should be on pass/fail check for a theme, rather than run is successfully completed or not. And exit code should be returned based on that I believe.
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.
I think I could implement your feedback, please see: #458 (comment)