Feature/change the redis nodejs library #14
Conversation
|
Thanks for the PR, @codeDude64 - we''ll take a look :) |
simonprickett
left a comment
simonprickett
left a comment
There was a problem hiding this comment.
Looking good and works locally when tested - I left one suggestion to retain the capability to connect to Redis at a configurable host/port/user/password... please take a look at that then we should be good to go. Thanks!
nodejs/average.js
Outdated
| const db = new Database(constants.sqlite_database) | ||
| const redis = new Redis(constants.redis) | ||
| const db = new Database(constants.sqlite_database); | ||
| const client = createClient(); |
There was a problem hiding this comment.
@codeDude64 this will always assume Redis is on localhost:6379 with no password. The previous ioredis code used the object redis in constants.js to specify where Redis is when connecting to it... please change this code here to:
const client = createClient({ url: constants.redis });
then update constants.redis so that the value of redis is a Redis URL, for example redis://localhost:6379 - this will allow people to configure where Redis is and optionally a username and password if needed in one line. See https://github.com/redis/node-redis/blob/master/docs/client-configuration.md for details.
There was a problem hiding this comment.
@codeDude64 this will always assume Redis is on localhost:6379 with no password. The previous ioredis code used the object
redisinconstants.jsto specify where Redis is when connecting to it... please change this code here to:const client = createClient({ url: constants.redis });then update
constants.redisso that the value ofredisis a Redis URL, for exampleredis://localhost:6379- this will allow people to configure where Redis is and optionally a username and password if needed in one line. See https://github.com/redis/node-redis/blob/master/docs/client-configuration.md for details.
Just to confirm the username and the password are optional in the Redis URL, right?
codeDude64
requested review from
simonprickett
and removed request for
SuzeShardlow
|
@codeDude64 I am curious as to what's going on here... who is @SalvadorJJ? Because they seem to have made all the commits on your PR. |
OMG So sorry for that. It's my another account. @SalvadorJJ It's the account that I use to work, I'm a software developer in a company, so I like to separate my personal projects with the work. I totally missed change the email in my local git configuration. I'll fix immediately |
codeDude64
force-pushed
the
feature/change-the-redis-nodejs-library
branch
from
1087205 to
e12de25
Compare
|
@SuzeShardlow I just fixed it. I changed the commits metadata with my correct personal email and username. |
simonprickett
left a comment
simonprickett
left a comment
There was a problem hiding this comment.
Thanks - looks good @SuzeShardlow will merge.
Thanks, no problem, I was just super confused and thought you were collaborating with someone else. You were collaborating with yourself! 😂 |
3 participants
Closes #13
##Proposal
My discords account is codeDude#4396