Skip to content

Fix handling for empty arguments in commands that throw server-side error #941

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

Closed
wants to merge 1 commit into from

Conversation

tzickel
Copy link

@tzickel tzickel commented Dec 25, 2017

Sometimes you pass an array to mget as an input, if the array is empty, instead of returning an empty array, mget throws an exception from the server (rightfully so) but it's not very pythonic.

I have lots of places in the code which call mget with an array that might or might not be empty, since in the end i just iterate over the result, adding an if not empty check before each mget call is superfluous (and if someone really wants to check it, he can).

@itamarhaber
Copy link
Member

Break early makes sense to me, and the change is sane. I don't know if the suggested is more pythonic, but it is certainly a nicer UX.

However, I'm worried about backward compatibility as an exception is currently thrown. I'd actually change the return statement to an exception, although I understand this does not address your pain.

@andymccurdy
Copy link
Contributor

This breaks pipelines. If you pass an empty list to MGET within a pipeline, you'll never see that response in the pipeline's execute because the MGET request won't get queued.

@tzickel
Copy link
Author

tzickel commented Nov 3, 2018

A. it's nice to see the project is back to life, hopefully my backlog of PR/issues will be handled one day :)
B. Correct, I can fix it to work with pipelines and maybe be more generic (MGET isn't the only function which behaves like this) if this is something you will be considering adopting in the code, say so and I'll make a better PR.

@andymccurdy
Copy link
Contributor

If it can be made to work with pipelines, I think returning an empty list is OK.

@tzickel tzickel force-pushed the mgetfix branch 2 times, most recently from 52d6251 to 9c0983c Compare November 3, 2018 19:57
@tzickel
Copy link
Author

tzickel commented Nov 3, 2018

@andymccurdy something like this ? this is more generic, you can see the changes now are for MGET.

@tzickel tzickel changed the title Fix handling for empty arguments in mget Fix handling for empty arguments in commands that throw server-side error Nov 3, 2018
@tzickel tzickel force-pushed the mgetfix branch 2 times, most recently from fb63709 to 2230500 Compare November 3, 2018 21:12
@andymccurdy
Copy link
Contributor

I created a different implementation #1052

IMHO I think it's a little cleaner.

@andymccurdy
Copy link
Contributor

I merged #1052 as I preferred its implementation to this one. It only affected MGET and we'll need a separate PR to add support for other commands with similar behavior, such as DEL.

@andymccurdy andymccurdy closed this Nov 5, 2018
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.

3 participants