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

Add support for transparent key prefixing. Close #95 #105

Merged
merged 4 commits into from
Jul 23, 2015

Conversation

dguo
Copy link
Contributor

@dguo dguo commented Jul 21, 2015

No description provided.

@@ -43,6 +43,11 @@ function Command(name, args, options, callback) {
this.replyEncoding = options && options.replyEncoding;
this.errorStack = options && options.errorStack;
this.args = args ? _.flatten(args) : [];
if (options && options.keyPrefix) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is costly to access options + keyPrefix each time, imo it shoud be cached in a var. Also options should be always an object?

Copy link
Collaborator

Choose a reason for hiding this comment

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

There's no place we can cache the keyPrefix option. options is possible to be undefined since it's optional. We may put

if (typeof options === 'undefined') {
  options = {};
}

at the top of the constructor so that we won't need to test options everytime.

@luin
Copy link
Collaborator

luin commented Jul 22, 2015

Awesome! Just checked out the changes and they're really nice! I'm going to do more tests and we'll soon have this feature. 🍺

@dguo
Copy link
Contributor Author

dguo commented Jul 22, 2015

Okay, I made the two changes.

this.args = args ? _.flatten(args) : [];
if (options.keyPrefix) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

should be:

var keyPrefix = options.keyPrefix;
if (keyPrefix) {
  this._iterateKeys(function (key) {
    return keyPrefix + key;
  });
}

Otherwise each iteration it will look for a pointer from options to .keyPrefix and we want to avoid it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Thanks.

luin added a commit that referenced this pull request Jul 23, 2015
Add support for transparent key prefixing. Close #95
@luin luin merged commit 3f9014c into redis:master Jul 23, 2015
@luin
Copy link
Collaborator

luin commented Jul 23, 2015

Released in 1.7.0 🍺

@dguo
Copy link
Contributor Author

dguo commented Jul 23, 2015

😄 Awesome! Thanks for the quick responses.

@dguo dguo deleted the key_prefixing branch July 23, 2015 12:27
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