Skip to content

Add count to post-type and taxonomy commands#241

Merged
schlessera merged 2 commits intowp-cli:masterfrom
atanas-dev:4573-add-count-to-post-type-and-taxonomy
Mar 16, 2019
Merged

Add count to post-type and taxonomy commands#241
schlessera merged 2 commits intowp-cli:masterfrom
atanas-dev:4573-add-count-to-post-type-and-taxonomy

Conversation

@atanas-dev
Copy link
Contributor

@atanas-dev atanas-dev 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-dev atanas-dev requested a review from a team as a code owner March 13, 2019 21:14
@atanas-dev
Copy link
Contributor Author

atanas-dev 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.

@atanas-dev atanas-dev mentioned this pull request Mar 13, 2019
Closed
@schlessera
Copy link
Member

schlessera 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.

@schlessera schlessera added command:post-type-list Related to 'post-type list' command command:taxonomy-list Related to 'taxonomy list' command labels Mar 14, 2019
@atanas-dev atanas-dev force-pushed the 4573-add-count-to-post-type-and-taxonomy branch 3 times, most recently from 1c5ed14 to 8bf4d0c Compare March 14, 2019 20:09
@atanas-dev
Copy link
Contributor Author

atanas-dev 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
schlessera requested changes Mar 16, 2019
View reviewed changes
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 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.

@atanas-dev atanas-dev force-pushed the 4573-add-count-to-post-type-and-taxonomy branch from 8bf4d0c to c561e95 Compare March 16, 2019 22:05
@atanas-dev
Copy link
Contributor Author

atanas-dev 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@schlessera schlessera schlessera approved these changes

Assignees

No one assigned

Labels

command:post-type-list Related to 'post-type list' command command:taxonomy-list Related to 'taxonomy list' command

Projects

None yet

Milestone

2.0.3

Development

Successfully merging this pull request may close these issues.

2 participants

@atanas-dev @schlessera