Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Separate non-bundled install; generate against current WP-CLI instance. #207

Merged
merged 4 commits into from Feb 19, 2018

Conversation

2 participants
@gitlost
Copy link
Contributor

gitlost commented Feb 9, 2018

See #200 (comment)

Adds a separate bin/install_packages.sh script to be run before running the handbook commands that installs the 4 non-bundled packages in bin/packages. The handbook gen-commands command then just checks that they're installed.

Uses the current WP-CLI instance to generate the doc against rather than invoking a separate instance and insists that it is run with WP_CLI_PACKAGES_DIR=bin/packages, and warns if isn't run with WP_CLI_CONFIG_PATH=/dev/null.

Empties the commands/ and internal-api/ directories first to allow for removed commands/apis. This causes a change in the currently generated doc as Utils\wp_clear_object_cache() was deprecated for v1.5.0 so isn't listed.

Adds --verbose option to gen-all and gen-commands to only output individual command listings if set, to make output simpler.

Changes update_commands_data() to take $full rather than $parent arg to simplify calling, and to just check when_invoke closure use arg is array or callable function (otherwise was getting an exception as can be set 2 both ways in CommandFactory::create_subcommand()), and sets repo_url in subcommand loop on first subcommand found if not already set, and only sets in $command_data array if something to set.

Awards "Hack of the Codebase" 馃 to @danielbachhuber for getting closure use clause arg with ReflectionFunction::getStaticVariables(). Masterful!

Edit: undid 2d24dff as need to check if array (with class/method) or function as the PR originally did.

@gitlost gitlost added this to the 2.0.0 milestone Feb 9, 2018

@gitlost gitlost requested a review from wp-cli/committers Feb 9, 2018

@schlessera

This comment has been minimized.

Copy link
Member

schlessera commented Feb 14, 2018

Awards "Hack of the Codebase" 馃 to @danielbachhuber for getting closure use clause arg with ReflectionFunction::getStaticVariables(). Masterful!

Speaking about hacks... => https://wordpress.slack.com/archives/C02RP4T41/p1502198773228898
Just be glad you don't have regexes in there now... ;)

public function gen_all() {
public function gen_all( $args, $assoc_args ) {
// Warn if not invoked with null WP_CLI_CONFIG_PATH.
if ( '/dev/null' !== getenv( 'WP_CLI_CONFIG_PATH' ) ) {

This comment has been minimized.

@schlessera

schlessera Feb 14, 2018

Member

That won't work on Windows. However, not sure if we need to be able to generate the docs on Windows machines...

This comment has been minimized.

@gitlost

gitlost Feb 14, 2018

Author Contributor

I did actually consider that also believe it or not - only a warning but it is lazy...

@schlessera
Copy link
Member

schlessera left a comment

Approving this, with the note that I think all of the extra steps (setting 2 env. variables & installing packages) could just as well be automated if they are a hard requirement.

public function gen_all() {
public function gen_all( $args, $assoc_args ) {
// Warn if not invoked with null WP_CLI_CONFIG_PATH.
if ( '/dev/null' !== getenv( 'WP_CLI_CONFIG_PATH' ) ) {

This comment has been minimized.

@schlessera

schlessera Feb 14, 2018

Member

If this is a requirement, why not just putenv() it to force it?

This comment has been minimized.

@gitlost

gitlost Feb 14, 2018

Author Contributor

I did actually consider that, but these are used during bootstrapping so there's no hook available that early? Plus there's an advantage I think to just being explicit and dumb so it's obvious what is happening for such a seldom used command....

$bundled_cmds[] = $cmd['name'];
public function gen_commands( $args, $assoc_args ) {
// Check invoked with packages directory set to `bin/packages'.
if ( ! preg_match( '/bin\/packages\/?$/', getenv( 'WP_CLI_PACKAGES_DIR' ) ) ) {

This comment has been minimized.

@schlessera

schlessera Feb 14, 2018

Member

If this is a requirement, why not just putenv() it to force it?

@schlessera schlessera merged commit d086616 into master Feb 19, 2018

@schlessera schlessera deleted the gen_commands-refactor branch Feb 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.