-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Conversation
@@ -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) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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. 🍺 |
Okay, I made the two changes. |
this.args = args ? _.flatten(args) : []; | ||
if (options.keyPrefix) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Thanks.
Add support for transparent key prefixing. Close #95
Released in 1.7.0 🍺 |
😄 Awesome! Thanks for the quick responses. |
No description provided.