-
Notifications
You must be signed in to change notification settings - Fork 41
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
store revision list in keyPrefix:revisions
#32
store revision list in keyPrefix:revisions
#32
Conversation
@@ -11,10 +12,12 @@ module.exports = CoreObject.extend({ | |||
set: function() { }, | |||
del: function() { }, | |||
zadd: function(key, score , tag) { | |||
this.recentRevisions.push(key + ':' + tag); | |||
assert(key.match(/:revisions$/)); |
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.
@achambers @lukemelia I'm doing an assertion for this in the fake client.. it's not super nice and not even needed but given that we stub redis entirely it seemed the most appropriate place...
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.
yeah, this is kind of ugly. maybe you can extend the fake client with a method that has the assert for just the test that cares about 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.
yeah, as mentioned, I agree..
it spans across a bunch of tests, but we already reopen the redis client in some tests anyway so it should do no harm..
will ping you shortly with an update
9c7ddc8
to
f82ede2
Compare
@lukemelia ping, let me know if this sounds better |
f82ede2
to
47f409e
Compare
}, | ||
zrem: function(val,revision) { | ||
var i = this.recentRevisions.indexOf(revision) | ||
zrem: function(key,revision) { |
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.
add space after comma
One tiny nit, otherwise, looks good! |
47f409e
to
3563b16
Compare
@lukemelia done! |
3563b16
to
1999ef8
Compare
also updated the README since we know list the keys there as well.. |
store revision list in `keyPrefix:revisions`
👍 |
fix #28