-
Notifications
You must be signed in to change notification settings - Fork 35
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 config is-true
command
#161
Add wp config is-true
command
#161
Conversation
…or not a variable is true.
…MSP-580-is-true-command # Conflicts: # features/config-is-true.feature # src/Config_Command.php
…rformance improvement.
Co-authored-by: Phil Tyler <philip@tylerdigital.com>
As this proces was quit interesting to follow 😁, here is the Slack thread to optimize the running time of the Behat test |
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.
Great start! Just a few comments to chat through. It seems generally solid otherwise.
* - constant | ||
* - variable | ||
* - all | ||
* --- |
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.
Should we offer a --strict
flag too, for strict 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.
I'm legitimately not sure what could be made stricter than it already is.
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.
Sorry, to clarify, --strict
would only WP_CLI::halt( 0 );
for define( 'MULTISITE', true );
. It would WP_CLI::halt( 1 );
for any truthy value (e.g. define( 'MULTISITE', 'true' );
).
We can also leave things as they are, and wait for that to come up as a request.
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 know that that's particularly valuable until someone asks for it. true/false and 0/1 will already be mostly interchangeable depending on programmer's habit.
@danielbachhuber Ready for re-review at your convience. |
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.
Looks great! Thanks for your work on this, @rwagner00
Resolves #159 by providing
config is-true
command along with related automated tests.It creates a protected method
get_value()
to prevent code duplication between the existingget()
and newis_true()
methods.Contributors: @pwtyler @rwagner00 @schlessera