Skip to content
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

Fix RedisClient instantiation for authentication #785

Merged
merged 1 commit into from
Aug 21, 2014
Merged

Fix RedisClient instantiation for authentication #785

merged 1 commit into from
Aug 21, 2014

Conversation

yamadapc
Copy link
Contributor

This removes the "INFO" command from the default RedisClient
constructor, since its only there to support the redisVersion getter.
This fixes creating clients for databases that require authentication
for all commands. It moves the version fetching logic into the
redisVersion getter, and caches its result in the m_version
property. I think this is the cleanest solution to this issue.

This removes the "INFO" command from the default `RedisClient`
constructor, since its only there to support the `redisVersion` getter.
This fixes creating clients for databases that require authentication
for all commands. It moves the version fetching logic into the
`redisVersion` getter, and caches its result in the `m_version`
property. I think this is the cleanest solution to this issue.
yamadapc added a commit to yamadapc/dmin.io that referenced this pull request Aug 19, 2014
This currently depends on vibe-d/vibe.d#785 and is required
for us to deploy this to Heroku.
@yamadapc
Copy link
Contributor Author

Can I get some feedback on this? It makes deploying vibe.d applications which use redis to heroku impossible. Maybe I missed something, but this is a pretty big bug.

@yamadapc
Copy link
Contributor Author

I removed a line I shouldn't have, if you want I'll push force and update the commit to be prettier. The ugly diff (because of moving code from a function to another) can't really be avoided. I'm sorry about it too.

@etcimon
Copy link
Contributor

etcimon commented Aug 21, 2014

It basically makes m_version lazily loaded. Looks good to me

@s-ludwig
Copy link
Member

Looks good indeed. Merging. (Sorry, I completely missed this pull request before)

s-ludwig added a commit that referenced this pull request Aug 21, 2014
Fix `RedisClient` instantiation for authentication
@s-ludwig s-ludwig merged commit d1dd6c9 into vibe-d:master Aug 21, 2014
@yamadapc
Copy link
Contributor Author

Awesome! Thanks :)

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.

3 participants