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

Abstract meta CRUD into methods #174

Merged
merged 8 commits into from May 15, 2018

Conversation

@spacedmonkey
Contributor

spacedmonkey commented Apr 16, 2018

In this PR, I abstract off the meta crud into there own method. This means that can overridden from the parent class.

fixes #172
related: #158

Show outdated Hide outdated src/WP_CLI/CommandWithMeta.php
Show outdated Hide outdated src/Comment_Meta_Command.php
Show outdated Hide outdated src/Term_Meta_Command.php
@schlessera

This comment has been minimized.

Show comment
Hide comment
@schlessera

schlessera Apr 16, 2018

Member

Good approach, only a few style fixes and the removal of a random paste needed.

Member

schlessera commented Apr 16, 2018

Good approach, only a few style fixes and the removal of a random paste needed.

@spacedmonkey

This comment has been minimized.

Show comment
Hide comment
@spacedmonkey

spacedmonkey Apr 17, 2018

Contributor

I have added some basic comments to this. @schlessera let me know if you want me to change them, I am not great for writing comments. Otherwise, I am happy with the PR as it is. Don't think there needs more work on it.

This PR needs to be merged before, #159 as that PR will need to changed, to implement changes in this PR.

The tests seem to be failing on trunk, but I think that is unreleated to my change...

Contributor

spacedmonkey commented Apr 17, 2018

I have added some basic comments to this. @schlessera let me know if you want me to change them, I am not great for writing comments. Otherwise, I am happy with the PR as it is. Don't think there needs more work on it.

This PR needs to be merged before, #159 as that PR will need to changed, to implement changes in this PR.

The tests seem to be failing on trunk, but I think that is unreleated to my change...

@schlessera

This comment has been minimized.

Show comment
Hide comment
@schlessera

schlessera Apr 17, 2018

Member

Ah, it seems to break on trunk now because a default Privacy Policy page was added as part of the GDPR effort.

I'll fix the tests for that.

Member

schlessera commented Apr 17, 2018

Ah, it seems to break on trunk now because a default Privacy Policy page was added as part of the GDPR effort.

I'll fix the tests for that.

@schlessera

This now contains lots of files from your IDE, please remove them from the pull request.

I recommend adding the .idea folder to your global git ignore list.

@spacedmonkey

This comment has been minimized.

Show comment
Hide comment
@spacedmonkey

spacedmonkey Apr 17, 2018

Contributor

My bad. That is a silly make. Sorry, added .idea to gitignore file.

Contributor

spacedmonkey commented Apr 17, 2018

My bad. That is a silly make. Sorry, added .idea to gitignore file.

@schlessera

This comment has been minimized.

Show comment
Hide comment
@schlessera

schlessera Apr 17, 2018

Member

@spacedmonkey I actually meant for you to add .idea to your own, global .gitignore file, not the project's one. The project's .gitignore should not need to care about the .idea folder, that is client-specific tooling.

See https://help.github.com/articles/ignoring-files/#create-a-global-gitignore

Member

schlessera commented Apr 17, 2018

@spacedmonkey I actually meant for you to add .idea to your own, global .gitignore file, not the project's one. The project's .gitignore should not need to care about the .idea folder, that is client-specific tooling.

See https://help.github.com/articles/ignoring-files/#create-a-global-gitignore

@spacedmonkey

This comment has been minimized.

Show comment
Hide comment
@spacedmonkey

spacedmonkey Apr 23, 2018

Contributor

@schlessera Tests are passing and feedback done.

Contributor

spacedmonkey commented Apr 23, 2018

@schlessera Tests are passing and feedback done.

@schlessera

This comment has been minimized.

Show comment
Hide comment
@schlessera

schlessera May 8, 2018

Member

@spacedmonkey I pushed a change to add full docblocks to the methods.

While doing so, I noticed that the functions delete_comment_meta(), delete_post_meta(), delete_term_meta() and delete_user_meta() only accept 3 arguments, not the 4 that are currently being passed.

Currently, the WP_CLI\CommandWithMeta::delete_metadata() method has a fourth argument $delete_all = false which is accepted by the general delete_metadata() function, but not by the specialized versions.

Reading through the docs, it seems to me that this argument does not make any sense in our current context, and would be more useful for a separate command.

How do you suggest we proceed?

Member

schlessera commented May 8, 2018

@spacedmonkey I pushed a change to add full docblocks to the methods.

While doing so, I noticed that the functions delete_comment_meta(), delete_post_meta(), delete_term_meta() and delete_user_meta() only accept 3 arguments, not the 4 that are currently being passed.

Currently, the WP_CLI\CommandWithMeta::delete_metadata() method has a fourth argument $delete_all = false which is accepted by the general delete_metadata() function, but not by the specialized versions.

Reading through the docs, it seems to me that this argument does not make any sense in our current context, and would be more useful for a separate command.

How do you suggest we proceed?

@spacedmonkey

This comment has been minimized.

Show comment
Hide comment
@spacedmonkey

spacedmonkey May 8, 2018

Contributor

Thanks for the feedback @schlessera !

I have remove the delete_all flag in 3e480f4 . It doesn't map to the arguments of the object type functions. There is already functionality for deleting all meta of a object, by passing --all flag to delete meta command, so I don't see the value of having another delete all sub command.

Contributor

spacedmonkey commented May 8, 2018

Thanks for the feedback @schlessera !

I have remove the delete_all flag in 3e480f4 . It doesn't map to the arguments of the object type functions. There is already functionality for deleting all meta of a object, by passing --all flag to delete meta command, so I don't see the value of having another delete all sub command.

@schlessera schlessera merged commit d44e0bf into wp-cli:master May 15, 2018

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