search

vapor / redis

Vapor provider for RediStack
MIT License
459 stars 58 forks source link

Fixed bug with makeConnection #97

Closed John-Connolly closed 6 years ago

John-Connolly commented 6 years ago

The result of the auth command probably should not be ignored.

pedantix commented 6 years ago

These surfaces are not tested, and Auth is particularly hard to test because of the way you configure Redis to use Auth.

I am opposed to untested surfaces being merged in as a rule, but "that is just my opinion man". I support the concept of this PR but oppose its state in the form of testing. I perhaps should have in retrospect added a throw RedisError to auth on failure so it is more clear what happens on failure. To me, it is not immediately obvious what happens if Auth fails. Could we elucidate what happens on failure if not with a test then with comments?

John-Connolly commented 6 years ago

Well before the error was being ignored if auth was unsuccessful at least now if auth is not successful there is a way of knowing before sending more commands to redis.

John-Connolly commented 6 years ago

I not sure my change is entirely correct, I don't use the RedisDatabase or the built in RedisProvider so I don't know when the best time to catch this error would be.

pedantix commented 6 years ago

my real question is what error is being trown on failure? this is one thing that could definately be tested

John-Connolly commented 6 years ago

The error would be whatever redis returns when auth is unsuccessful

pedantix commented 6 years ago

oh yeh no doubt but what class of error would get thrown? RedisError, VaporError I am just confused on that part

John-Connolly commented 6 years ago

Well, currently its the redis error that gets thrown. I don't know where it bubbles up to. I am going to close this because its probably not the right solution.