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

implement Copy command #242

Closed

Conversation

matiasinsaurralde
Copy link
Contributor

@matiasinsaurralde matiasinsaurralde commented Jan 9, 2022

First attempt at implementing Copy command (#241), still need to polish some stuff

@alicebob
Copy link
Owner

alicebob commented Jan 9, 2022

Hi,

thanks!, looks good in general. You'll also want to add some tests to ./integration/generic_test.go, which will likely help you to get the error cases correctly.

@matiasinsaurralde
Copy link
Contributor Author

Hi,

thanks!, looks good in general. You'll also want to add some tests to ./integration/generic_test.go, which will likely help you to get the error cases correctly.

Great, I will look into that during the week.

Thanks for the feedback.

@matiasinsaurralde
Copy link
Contributor Author

matiasinsaurralde commented Jan 11, 2022

@alicebob I've added initial test cases which should cover the most basic functionality of the copy command. Have also fixed the output checks in both integration and standard tests as this command doesn't really return errors (like no such key) but just simple integer values for each scenario.

I believe that a separate PR could be prepared for supporting the two optional arguments (db and replace), as described here.

Test output:

% cd integration ; go test . -v -tags 'int' -run Copy
=== RUN   TestCopy
--- PASS: TestCopy (0.02s)
PASS
ok  	github.com/alicebob/miniredis/v2/integration	(cached)
% redis-cli info | grep redis_version
redis_version:6.2.6
% go test . -v -run Copy
=== RUN   TestCopy
=== RUN   TestCopy/nonexistent_key
=== RUN   TestCopy/existing_key
--- PASS: TestCopy (0.00s)
    --- PASS: TestCopy/nonexistent_key (0.00s)
    --- PASS: TestCopy/existing_key (0.00s)
PASS
ok  	github.com/alicebob/miniredis/v2	(cached)

@matiasinsaurralde matiasinsaurralde force-pushed the copy-command branch 2 times, most recently from 4a40a5e to 5052bda Compare January 11, 2022 05:15
@alicebob
Copy link
Owner

thanks! I'll have a look soon.

@alicebob
Copy link
Owner

ah, I forgot, can you squash the commits? That's easier for me to merge them later. Thanks!

@matiasinsaurralde
Copy link
Contributor Author

@alicebob Squashed 👍

@rockitbaby
Copy link

Thank you for contributing @matiasinsaurralde – this looks good to me!
Minor: Do not forget to add COPY to the list of implemented commands in README.md

@matiasinsaurralde
Copy link
Contributor Author

@rockitbaby Thanks for the suggestion, have updated the README

@alicebob alicebob mentioned this pull request Jan 12, 2022
@alicebob
Copy link
Owner

alicebob commented Jan 12, 2022

Hi! thanks for the last fix. I was just finishing this. I've added support for DB and REPLACE in #244. But the DB argument wouldn't work with the existing implementation, so I've changed the m.Copy() to be m.Copy(0, "from", 0, "to") (or whichever database IDs you want). Bit verbose, but at least it's clear.
There was a serious bug that it didn't copy the contents of keys (such as hashes), but only the pointer, which means that changing the old key would also change the copy. That's also fixed.

@matiasinsaurralde
Copy link
Contributor Author

@alicebob Thanks, looking good

@alicebob
Copy link
Owner

thanks! it's merged in master

@alicebob alicebob closed this Jan 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants