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 pluck and patch subcommands #24

Merged
merged 33 commits into from Aug 11, 2017

Conversation

3 participants
@aaemnnosttv
Contributor

aaemnnosttv commented Jul 5, 2017

Hey guys,

I thought I would post this here for some feedback before I go on and do much more.

I've started with the meta commands as it seemed like the easiest way to affect the most commands.

I added some things like PHPUnit which helped a lot with testing the class I added; that OK?

I expect some things will need to change but let me know if we're on the right track here, or if you'd like me to be doing something different while things are more flexible.

Resolves wp-cli/ideas#42

@danielbachhuber

Great start! Left a few comments inline

Show outdated Hide outdated features/post-meta.feature Outdated
"""
When I try `wp post meta pluck 1 meta-key foo`
Then STDOUT should be empty

This comment has been minimized.

@danielbachhuber

danielbachhuber Jul 6, 2017

Member

We should set an explicit check for return code here too:

And the return code should be 1
@danielbachhuber

danielbachhuber Jul 6, 2017

Member

We should set an explicit check for return code here too:

And the return code should be 1
Show outdated Hide outdated src/WP_CLI/CommandWithMeta.php Outdated
Show outdated Hide outdated src/WP_CLI/CommandWithMeta.php Outdated
Show outdated Hide outdated bin/test.sh Outdated
@aaemnnosttv

This comment has been minimized.

Show comment
Hide comment
@aaemnnosttv

aaemnnosttv Jul 9, 2017

Contributor

Commands have been updated to use positional args as the target key path, and the concept of a delimiter has been removed. I also updated the assertions to use JSON for consistency with the input.

We still need to come to a decision as to how unset should be used either as a patch flag or separate command. I found a similar case which we will need to make a similar decision for in the case of wanting to set a value for a non-existent key.

By default, the command errors out if the key does not exist. If we want to create the key with the value (ie. insert it) then we need a way to set that in the command.

This is where I am looking for feedback from you guys. I'm not a big fan of using/keeping the mode flag, but I also don't really like the idea of adding 2 more named subcommands which are slightly different than patch, particularly because I can't think of any good names for the operations that wouldn't be confused for other crud commands. unset could be confused with delete, and something like insert could be confused with add. Thoughts?

Contributor

aaemnnosttv commented Jul 9, 2017

Commands have been updated to use positional args as the target key path, and the concept of a delimiter has been removed. I also updated the assertions to use JSON for consistency with the input.

We still need to come to a decision as to how unset should be used either as a patch flag or separate command. I found a similar case which we will need to make a similar decision for in the case of wanting to set a value for a non-existent key.

By default, the command errors out if the key does not exist. If we want to create the key with the value (ie. insert it) then we need a way to set that in the command.

This is where I am looking for feedback from you guys. I'm not a big fan of using/keeping the mode flag, but I also don't really like the idea of adding 2 more named subcommands which are slightly different than patch, particularly because I can't think of any good names for the operations that wouldn't be confused for other crud commands. unset could be confused with delete, and something like insert could be confused with add. Thoughts?

@danielbachhuber

This comment has been minimized.

Show comment
Hide comment
@danielbachhuber

danielbachhuber Jul 10, 2017

Member

This is where I am looking for feedback from you guys. I'm not a big fan of using/keeping the mode flag, but I also don't really like the idea of adding 2 more named subcommands which are slightly different than patch, particularly because I can't think of any good names for the operations that wouldn't be confused for other crud commands.

This is my sentiment too. I don't have a strong opinion one way or another at this point (--mode=<mode> argument vs. separate subcommands).

Another idea would be: wp option patch (add|remove|update|delete).

Member

danielbachhuber commented Jul 10, 2017

This is where I am looking for feedback from you guys. I'm not a big fan of using/keeping the mode flag, but I also don't really like the idea of adding 2 more named subcommands which are slightly different than patch, particularly because I can't think of any good names for the operations that wouldn't be confused for other crud commands.

This is my sentiment too. I don't have a strong opinion one way or another at this point (--mode=<mode> argument vs. separate subcommands).

Another idea would be: wp option patch (add|remove|update|delete).

@danielbachhuber

This comment has been minimized.

Show comment
Hide comment
@danielbachhuber

danielbachhuber Jul 10, 2017

Member

How does this work with numerically indexed values?

Member

danielbachhuber commented Jul 10, 2017

How does this work with numerically indexed values?

@aaemnnosttv

This comment has been minimized.

Show comment
Hide comment
@aaemnnosttv

aaemnnosttv Jul 10, 2017

Contributor

Another idea would be: wp option patch (add|remove|update|delete).

I like this the most out of all the options so far 👍

How does this work with numerically indexed values?

Good question! The argument will be interpreted as a string I believe and will check for a key with a loose comparison. The traverser can obviously handle any valid type as keys to navigate, the limitation is more on taking the input from the command args where the type will have to be inferred. I don't think there will be a perfect solution here, but I think the only situation where it would be problematic is if a key existed for both the string and numeric value.

E.g.

array(
 1 => 'int one',
 '1' => 'string one',
)

This seems like an edge case that isn't worth handling, but it is something we should look into and make sure that it works reasonably well with numeric keys.

I'll add some tests for this and keep this in mind when building out add/insert.

Contributor

aaemnnosttv commented Jul 10, 2017

Another idea would be: wp option patch (add|remove|update|delete).

I like this the most out of all the options so far 👍

How does this work with numerically indexed values?

Good question! The argument will be interpreted as a string I believe and will check for a key with a loose comparison. The traverser can obviously handle any valid type as keys to navigate, the limitation is more on taking the input from the command args where the type will have to be inferred. I don't think there will be a perfect solution here, but I think the only situation where it would be problematic is if a key existed for both the string and numeric value.

E.g.

array(
 1 => 'int one',
 '1' => 'string one',
)

This seems like an edge case that isn't worth handling, but it is something we should look into and make sure that it works reasonably well with numeric keys.

I'll add some tests for this and keep this in mind when building out add/insert.

@danielbachhuber

This comment has been minimized.

Show comment
Hide comment
@danielbachhuber

danielbachhuber Jul 10, 2017

Member

I'll add some tests for this and keep this in mind

Great, thanks. I don't think we need a solution for the ambiguous scenario either, but it would be good to have a test with explicit assertions of behavior.

Member

danielbachhuber commented Jul 10, 2017

I'll add some tests for this and keep this in mind

Great, thanks. I don't think we need a solution for the ambiguous scenario either, but it would be good to have a test with explicit assertions of behavior.

@aaemnnosttv

This comment has been minimized.

Show comment
Hide comment
@aaemnnosttv

aaemnnosttv Jul 16, 2017

Contributor

Update! Pluck and patch are functionally complete for meta commands 🎉

Patch now requires the action to be set as the first argument (insert|update|delete) which works quite nicely!

More tests have been added for everything, including the behavior with indexed keys.

I thought indexed keys would be more of a pain than they were, but it turns out we only needed to cast them to integers (when they look like one) and everything just works. I tried to create a scenario for the edge case where I was expecting this to fail using a string integer for the key, but found this quite hard to do. Both JSON and PHP convert string integers to integers when serializing.

serialize( ["0" => "foo"] )
// string(20) "a:1:{i:0;s:3:"foo";}"

Who knew!? 😁

The question now is whether this looks good enough to start replicating to the rest of the entities?
Also, I was thinking about namespaces. Should \WP_CLI\RecursiveDataStructureTraverser be moved under \WP_CLI\Entity\RecursiveDataStructureTraverser ? I realize that probably can't be done with the other classes in the package since they existed before the split, but it seems like it might be a good idea going forward.

Contributor

aaemnnosttv commented Jul 16, 2017

Update! Pluck and patch are functionally complete for meta commands 🎉

Patch now requires the action to be set as the first argument (insert|update|delete) which works quite nicely!

More tests have been added for everything, including the behavior with indexed keys.

I thought indexed keys would be more of a pain than they were, but it turns out we only needed to cast them to integers (when they look like one) and everything just works. I tried to create a scenario for the edge case where I was expecting this to fail using a string integer for the key, but found this quite hard to do. Both JSON and PHP convert string integers to integers when serializing.

serialize( ["0" => "foo"] )
// string(20) "a:1:{i:0;s:3:"foo";}"

Who knew!? 😁

The question now is whether this looks good enough to start replicating to the rest of the entities?
Also, I was thinking about namespaces. Should \WP_CLI\RecursiveDataStructureTraverser be moved under \WP_CLI\Entity\RecursiveDataStructureTraverser ? I realize that probably can't be done with the other classes in the package since they existed before the split, but it seems like it might be a good idea going forward.

@danielbachhuber danielbachhuber requested a review from schlessera Jul 17, 2017

@danielbachhuber

This comment has been minimized.

Show comment
Hide comment
@danielbachhuber

danielbachhuber Jul 17, 2017

Member

Update! Pluck and patch are functionally complete for meta commands

Sweet :) Looking good on my end. Given the amount of code landing, I'd like for @schlessera to give it a review too.

Should \WP_CLI\RecursiveDataStructureTraverser be moved under \WP_CLI\Entity\RecursiveDataStructureTraverser?

Yes, I think this would be a good idea.

I realize that probably can't be done with the other classes in the package since they existed before the split, but it seems like it might be a good idea going forward.

Correct, and sounds good.

Member

danielbachhuber commented Jul 17, 2017

Update! Pluck and patch are functionally complete for meta commands

Sweet :) Looking good on my end. Given the amount of code landing, I'd like for @schlessera to give it a review too.

Should \WP_CLI\RecursiveDataStructureTraverser be moved under \WP_CLI\Entity\RecursiveDataStructureTraverser?

Yes, I think this would be a good idea.

I realize that probably can't be done with the other classes in the package since they existed before the split, but it seems like it might be a good idea going forward.

Correct, and sounds good.

* options:
* - plaintext
* - json
* - yaml

This comment has been minimized.

@danielbachhuber

danielbachhuber Jul 17, 2017

Member

Could we get a couple useful ## EXAMPLES here?

@danielbachhuber

danielbachhuber Jul 17, 2017

Member

Could we get a couple useful ## EXAMPLES here?

This comment has been minimized.

@danielbachhuber
@danielbachhuber

danielbachhuber Aug 3, 2017

Member

@aaemnnosttv Bump here.

* options:
* - plaintext
* - json
* ---

This comment has been minimized.

@danielbachhuber

danielbachhuber Jul 17, 2017

Member

Could we get a couple useful ## EXAMPLES here?

@danielbachhuber

danielbachhuber Jul 17, 2017

Member

Could we get a couple useful ## EXAMPLES here?

This comment has been minimized.

@danielbachhuber
@danielbachhuber

danielbachhuber Aug 3, 2017

Member

@aaemnnosttv Bump here

@aaemnnosttv

This comment has been minimized.

Show comment
Hide comment
@aaemnnosttv

aaemnnosttv Jul 31, 2017

Contributor

Hey guys, I just pushed another update with many things addressed. One big improvement is that I found a way to reliably test if STDIN was passed or not which eliminates the possibility of the last argument being mistakenly used for the value if the STDIN is empty.

I added a method on a new Entity\Utils class for this although I'm guessing this could be useful for core as well. I thought we can start with it here and then just replace the import if/when it is assimilated into core?

I still need to add a few extra functional test cases that were requested, but I think we're really close to something that can be replicated to other commands.

Contributor

aaemnnosttv commented Jul 31, 2017

Hey guys, I just pushed another update with many things addressed. One big improvement is that I found a way to reliably test if STDIN was passed or not which eliminates the possibility of the last argument being mistakenly used for the value if the STDIN is empty.

I added a method on a new Entity\Utils class for this although I'm guessing this could be useful for core as well. I thought we can start with it here and then just replace the import if/when it is assimilated into core?

I still need to add a few extra functional test cases that were requested, but I think we're really close to something that can be replicated to other commands.

@schlessera

This comment has been minimized.

Show comment
Hide comment
@schlessera

schlessera Aug 1, 2017

Member

Yes, let's keep the function within Entity\Utils for now and only refactor when it is effectively needed elsewhere.

Member

schlessera commented Aug 1, 2017

Yes, let's keep the function within Entity\Utils for now and only refactor when it is effectively needed elsewhere.

aaemnnosttv and others added some commits Aug 1, 2017

@danielbachhuber

This comment has been minimized.

Show comment
Hide comment
@danielbachhuber

danielbachhuber Aug 3, 2017

Member

@aaemnnosttv Aside from the couple remaining small nits, are you happy enough with this to land it?

@schlessera Can you give this one final review?

Member

danielbachhuber commented Aug 3, 2017

@aaemnnosttv Aside from the couple remaining small nits, are you happy enough with this to land it?

@schlessera Can you give this one final review?

@danielbachhuber

This comment has been minimized.

Show comment
Hide comment
@danielbachhuber

danielbachhuber Aug 9, 2017

Member

@aaemnnosttv Aside from the couple remaining small nits, are you happy enough with this to land it?

Bump @aaemnnosttv

Member

danielbachhuber commented Aug 9, 2017

@aaemnnosttv Aside from the couple remaining small nits, are you happy enough with this to land it?

Bump @aaemnnosttv

@aaemnnosttv

This comment has been minimized.

Show comment
Hide comment
@aaemnnosttv

aaemnnosttv Aug 11, 2017

Contributor

@danielbachhuber I'm much happier with it now that it does not have the edge case that @schlessera mentioned. I'm guessing there may be a few small abstractions to make which might come up when replicating to the other commands, but I think it's in a good spot. Do you guys feel like it's functionally ready to start rolling it out to other entities?

Contributor

aaemnnosttv commented Aug 11, 2017

@danielbachhuber I'm much happier with it now that it does not have the edge case that @schlessera mentioned. I'm guessing there may be a few small abstractions to make which might come up when replicating to the other commands, but I think it's in a good spot. Do you guys feel like it's functionally ready to start rolling it out to other entities?

@schlessera

This comment has been minimized.

Show comment
Hide comment
@schlessera

schlessera Aug 11, 2017

Member

@aaemnnosttv Yes, I think the functionality is good now. Feel free to attack the other commands as well, while abstracting as needed to keep the code DRY.

Member

schlessera commented Aug 11, 2017

@aaemnnosttv Yes, I think the functionality is good now. Feel free to attack the other commands as well, while abstracting as needed to keep the code DRY.

@danielbachhuber danielbachhuber changed the title from Add pluck and patch subcommands [WIP] to Add pluck and patch subcommands Aug 11, 2017

@danielbachhuber danielbachhuber added this to the 1.1.0 milestone Aug 11, 2017

Ready to land

@danielbachhuber

This comment has been minimized.

Show comment
Hide comment
@danielbachhuber

danielbachhuber Aug 11, 2017

Member

@aaemnnosttv Let's go ahead and merge this then and handle the others in subsequent PRs.

Member

danielbachhuber commented Aug 11, 2017

@aaemnnosttv Let's go ahead and merge this then and handle the others in subsequent PRs.

@danielbachhuber danielbachhuber merged commit d0cd99c into wp-cli:master Aug 11, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@danielbachhuber

This comment has been minimized.

Show comment
Hide comment
@danielbachhuber

danielbachhuber Aug 17, 2017

Member

Created a new issue for wp option (patch|pluck) #46

@aaemnnosttv As far as I can think of, wp option * and wp * meta are the only two classes of commands where this would be helpful. Can you think of any others?

Member

danielbachhuber commented Aug 17, 2017

Created a new issue for wp option (patch|pluck) #46

@aaemnnosttv As far as I can think of, wp option * and wp * meta are the only two classes of commands where this would be helpful. Can you think of any others?

@aaemnnosttv

This comment has been minimized.

Show comment
Hide comment
@aaemnnosttv

aaemnnosttv Aug 17, 2017

Contributor

wp site option and wp transient would benefit as well, although Transients would require a PR to the wp-cli/cache-command package. Thoughts?

Contributor

aaemnnosttv commented Aug 17, 2017

wp site option and wp transient would benefit as well, although Transients would require a PR to the wp-cli/cache-command package. Thoughts?

@danielbachhuber

This comment has been minimized.

Show comment
Hide comment
@danielbachhuber

danielbachhuber Aug 17, 2017

Member

Good call re: wp site option. I don't think wp transient is necessary though... we can wait until someone identifies an actual need for it.

Member

danielbachhuber commented Aug 17, 2017

Good call re: wp site option. I don't think wp transient is necessary though... we can wait until someone identifies an actual need for it.

@aaemnnosttv aaemnnosttv deleted the aaemnnosttv:pr/pluck-patch-subcommands branch Aug 29, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment