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

stop mutating the arguments when calling multi #480

Merged
merged 1 commit into from
Jun 9, 2017

Conversation

reconbot
Copy link
Contributor

@reconbot reconbot commented Jun 5, 2017

This copies the arguments for a multi command instead of mutating the passed in arrays. This allows users to log the commands that gave an error if the multi command errors.

Copy link
Collaborator

@AVVS AVVS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perf hit, vars reinitialized, should move it out of loop

@reconbot
Copy link
Contributor Author

reconbot commented Jun 6, 2017

I manually hoisted the var declarations. Isn't that automatic?

@reconbot
Copy link
Contributor Author

reconbot commented Jun 6, 2017

I think I misunderstood you. Yes the vars are assigned in the loop. Are you saying that not using an intermediate variable (for readability and clarity) will improve the performance?

@luin
Copy link
Collaborator

luin commented Jun 8, 2017

Thank you for the pull request! There are two missing trailing commas. Could you add them?

This copies the arguments for a multi command instead of mutating the passed in arrays. This allows users to log the commands that gave an error if the multi command errors.
@reconbot
Copy link
Contributor Author

reconbot commented Jun 8, 2017

Fixed up the semicolons =)

@luin luin merged commit a380030 into redis:master Jun 9, 2017
@reconbot reconbot deleted the no-mutate-multi branch June 9, 2017 02:50
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