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

use passed redis connection object rather than create one #2

Merged
merged 5 commits into from
Oct 16, 2011
Merged

use passed redis connection object rather than create one #2

merged 5 commits into from
Oct 16, 2011

Conversation

atdt
Copy link
Contributor

@atdt atdt commented Oct 16, 2011

I think it's a bit awkward and wasteful that openid-redis insists on creating a redis.Redis connection instance. Presumably, if someone is using redis for Open ID authentication, s/he is using it for other things too, so s/he is likely to already have a connection set up. I think it's neater to just require a redis client as a parameter to init.

@bbangert
Copy link
Owner

It is a bit wasteful, though there's been a decent amount of ppl using this openid store for awhile, and just completely breaking the existing API in one go seems really bad. How about retaining the existing signature to avoid breaking everyone's apps, and making it optionally take a conn argument?

@atdt
Copy link
Contributor Author

atdt commented Oct 16, 2011

Yes, that sounds reasonable. I've made things backwards-compatible as you suggested. I'm still learning git, so please excuse the superfluous commits .

@bbangert
Copy link
Owner

Thanks!

@bbangert bbangert closed this Oct 16, 2011
bbangert added a commit that referenced this pull request Oct 16, 2011
use passed redis connection object rather than create one
@bbangert bbangert merged commit 8d805fe into bbangert:master Oct 16, 2011
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