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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add count to post-type and taxonomy commands #241

Conversation

@atanas-angelov-dev
Copy link
Contributor

commented Mar 13, 2019

Fixes wp-cli/wp-cli#4573.

I took the liberty of implementing the term count for the taxonomy command as well even though it was not mentioned in the original issue as it is in a very similar situation.

Feedback is very much welcome.

@atanas-angelov-dev atanas-angelov-dev requested a review from wp-cli/committers as a code owner Mar 13, 2019

@atanas-angelov-dev

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2019

The WP 3.7.11 test failed because the count for the page post type doesn't match up as only one page is created (Sample Page) vs 2 pages on latest (Sample Page and Privacy Policy).

I don't really have much experience with Behat so I'm not really sure what's the best approach here.

@schlessera

This comment has been minimized.

Copy link
Member

commented Mar 14, 2019

@atanas-angelov-dev First of all, thanks for the PR, This looks like a great start!

I'm still a bit wary of the performance impact of the count, so I suggest the following change:

  • Don't add the count column to the output by default (also for BC reasons).
  • Only execute the queries when the count column was actually included in the output using the --fields argument.

Regarding the failure on WP 3.7, I don't think we need to bother with testing the counts on there. Just add a @require-wp-5.0 and we're good to go. Note though that you'll have to provide a separate test for the count functionality once it's not part of the default output anymore.

@atanas-angelov-dev atanas-angelov-dev force-pushed the atanas-angelov-dev:4573-add-count-to-post-type-and-taxonomy branch 3 times, most recently from 1c5ed14 to 8bf4d0c Mar 14, 2019

@atanas-angelov-dev

This comment has been minimized.

Copy link
Contributor Author

commented Mar 14, 2019

@schlessera feedback has been addressed 👍

I had to limit the taxonomy tests to 5.0 as well since category count is 0 instead of 1 on 3.7.11 (guess Uncategorized is not created automatically under test conditions on that version or something?)

@schlessera
Copy link
Member

left a comment

This is great code, @atanas-angelov-dev, you seem to have covered all the bases.

I only have two minor nitpicks, then we can merge this:

  • Please use short array syntax, as we are at PHP 5.4+ now. I think I marked all the places with suggestions where this is needed, so you can just accept these.
  • For the descriptions for the 4 commands you modified, you should add a mention of the new optional field to the AVAILABLE OPTIONS section in the docblock. You can see an example of this in the docblock for the wp post list command: https://github.com/wp-cli/entity-command/blob/v2.0.2/src/Post_Command.php#L525-L527
    Note: The wp taxonomy get command even seems to be missing the entire AVAILABLE OPTIONS section, so please add that in one go now to be able to mention the optional field as well.

Once the above is changes, this can be merged.

src/Post_Type_Command.php Outdated Show resolved Hide resolved
src/Post_Type_Command.php Outdated Show resolved Hide resolved
src/Post_Type_Command.php Outdated Show resolved Hide resolved
src/Taxonomy_Command.php Outdated Show resolved Hide resolved
src/Taxonomy_Command.php Outdated Show resolved Hide resolved
src/Taxonomy_Command.php Outdated Show resolved Hide resolved

@atanas-angelov-dev atanas-angelov-dev force-pushed the atanas-angelov-dev:4573-add-count-to-post-type-and-taxonomy branch from 8bf4d0c to c561e95 Mar 16, 2019

@atanas-angelov-dev

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2019

@schlessera all done - I even ended up adjusting the default fields section for Taxonomy_Command::list_() as it was outdated.

@schlessera schlessera added this to the 2.0.3 milestone Mar 16, 2019

@schlessera schlessera merged commit a6e4c61 into wp-cli:master Mar 16, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants
You can’t perform that action at this time.