Skip to content

v.1.2 proposal #3

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

Merged
merged 8 commits into from
Apr 14, 2016
Merged

v.1.2 proposal #3

merged 8 commits into from
Apr 14, 2016

Conversation

BridgeAR
Copy link
Member

I added a build sanity check and added MIGRATE [...] KEYS key1, ... support.
Plus some small cleanup.


var Redis = require('ioredis');
var redis = new Redis(process.env.REDIS_URI);

redis.command(function (err, res) {
return redis.command().then(function (res) {
Copy link
Member

Choose a reason for hiding this comment

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

Why return is needed here?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is not. It's only my habit of adding a return in front of promises, if not explicitly unwanted.

@BridgeAR
Copy link
Member Author

Comments incorporated

@luin
Copy link
Member

luin commented Apr 13, 2016

Cool! And we'd better add some tests for migrate command:

expect(index('migrate', ['127.0.0.1', 6379, 'foo', 0, 0, 'COPY'])).to.eql([2]);
expect(index('migrate', ['127.0.0.1', 6379, '', 0, 0, 'REPLACE', 'KEYS', 'foo', 'bar'])).to.eql([7, 8]);
expect(index('migrate', ['127.0.0.1', 6379, '', 0, 0, 'KEYS', 'foo', 'bar'])).to.eql([6, 7]);

@BridgeAR
Copy link
Member Author

Tests added including a posttest for 100% coverage by adding istanbul.

While being on it I also updated ioredis and found that .getKeyIndexes() default had a check that was redundant and the other one could only be met by invalid argument length for that command. As this would result in an error anyway, I decided to remove that check.

@luin
Copy link
Member

luin commented Apr 13, 2016

image

@BridgeAR
Copy link
Member Author

Thanks for the review 😃

@BridgeAR BridgeAR merged commit 004f76d into master Apr 14, 2016
@BridgeAR BridgeAR deleted the v.1.2 branch April 21, 2016 10:30
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.

2 participants