Skip to content

Add --all flag to remove all terms from a post #23

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

Merged
merged 12 commits into from
Jul 10, 2017

Conversation

BhargavBhandari90
Copy link
Contributor

@BhargavBhandari90 BhargavBhandari90 commented Jul 1, 2017

Add parameter to remove all terms from the particular post

Fixes #18


// No need to specify terms while removing all terms.
if ( $terms ) {
WP_CLI::error( "No need to specify terms while removing all terms." );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for double quotes here.

if ( $terms ) {
$result = wp_remove_object_terms( $object_id, $terms, $taxonomy );
} else {
WP_CLI::error( "Please specify one or more terms." );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for double quotes here.

$cat_id = array( 1 );
$result = wp_set_object_terms( $object_id, $cat_id, $taxonomy, true );
} else {
if ( $terms ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Negate to bail early here to avoid the second else.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@schlessera I did a change here. Was this the thing which you want?

@schlessera
Copy link
Member

Please add relevant functional tests for this. See https://make.wordpress.org/cli/handbook/pull-requests/#running-and-writing-tests

@BhargavBhandari90
Copy link
Contributor Author

Please add relevant functional tests for this

I tried running behat test.
When I tried to run ./vendor/bin/behat --expand, then I am getting below error

[RuntimeException]
$ mkdir -p "C:\Users\bunty\AppData\Local\Temp/wp-cli-test-cache"
A subdirectory or file -p already exists.
Error occurred while processing: -p.
A subdirectory or file C:\Users\bunty\AppData\Local\Temp/wp-cli-test-cache already exists.
Error occurred while processing: C:\Users\bunty\AppData\Local\Temp/wp-cli-test-cache.
cwd:
exit status: 1

Am I doing something wrong?

@BhargavBhandari90
Copy link
Contributor Author

@schlessera All changes reported by you are done. Please review it.

Copy link
Member

@schlessera schlessera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not quite what I meant.

The general principle is that you want to reduce both indentation levels and the number of else branches that you have in your code. This helps reduce overally complexity, because your code ends up having less branches going on at the same time.

To achieve this, one popular and useful method is to bail early where it makes sense.

This can be done here by turning the code around.

So, instead of this:

} else {
   if ( $terms ) {
      $result = ...
   } else {
      WP_CLI::error( ... );
   }
}

we want to prefer this:

} else {
   if ( ! $terms ) {
      WP_CLI::error( ... );
   }
   $result = ...
}

This reduces the indentation level of the $result = ... statement, because we completed one possible branch above by exiting the code early if our condition is not met.

@danielbachhuber danielbachhuber changed the title Feature/gh #18 Add --all flag to remove all terms from a post Jul 4, 2017
@danielbachhuber danielbachhuber self-requested a review July 4, 2017 13:34
Copy link
Member

@danielbachhuber danielbachhuber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add functional tests please? Here's an introduction: https://make.wordpress.org/cli/handbook/pull-requests/

$result = wp_delete_object_term_relationships( $object_id, $taxonomy );

// Set post to default category.
$cat_id = array( 1 );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should only be assigned if the taxonomy is 'category'. Also, the default category is a configurable option we should respect.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is done

@BhargavBhandari90
Copy link
Contributor Author

@schlessera @danielbachhuber I have added the functional test for the feature. Please check this.


if ( 'category' === $taxonomy ) {
// Set post to default category.
$cat_id = array( 1 );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will need to use get_option( 'default_category' ), because this is a user-configurable value.

When I run `wp post term remove 1 category --all`
Then STDOUT should be:
"""
Success: Removed term.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we improve the message in this case to be:

Success: Removed all terms.

@@ -130,7 +133,32 @@ public function remove( $args, $assoc_args ) {
if ( $field = Utils\get_flag_value( $assoc_args, 'by' ) ) {
$terms = $this->prepare_terms( $field, $terms, $taxonomy );
}
$result = wp_remove_object_terms( $object_id, $terms, $taxonomy );

if ( $field = Utils\get_flag_value( $assoc_args, 'all' ) ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not necessary to define $field if you're not using it later.

"""
Success: Removed term.
"""

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also have a test that the default category was added back to the post.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added this. Please review this.

@BhargavBhandari90
Copy link
Contributor Author

@schlessera @danielbachhuber All changes are done. Please confirm this.

@danielbachhuber
Copy link
Member

@BhargavBhandari90 Thanks for your work on this!

@danielbachhuber danielbachhuber merged commit 253caa3 into wp-cli:master Jul 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants