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

Make FakeStrictRedis inherit redis.StrictRedis #36

Closed
wants to merge 1 commit into from
Closed

Make FakeStrictRedis inherit redis.StrictRedis #36

wants to merge 1 commit into from

Conversation

clee704
Copy link
Contributor

@clee704 clee704 commented Feb 3, 2014

Some libraries check whether the type of an argument is
redis.StrictRedis or not. If FakeStrictRedis is complete, this
inheritance should make no difference.

@jamesls
Copy link
Owner

jamesls commented Feb 12, 2014

As per the contribution guidelines this will need tests before I can merge this.

This should be a pretty easy test to write so I'll go ahead and work on this.

@jamesls
Copy link
Owner

jamesls commented Feb 12, 2014

Hmm, actually, in writing tests for this I'm not sure of the desired behavior. Can you elaborate on your use case? How would someone be using the isinstance checks here?

Does this also mean that FakeRedis should pass an isinstance check for redis.Redis?

@clee704
Copy link
Contributor Author

clee704 commented Feb 12, 2014

I was using FakeRedis with rq, which insisted the Queue should be passed a real Redis instance. Now I think it is quite trivial to workaround this, by making a class that inherits from both FakeRedis and Redis. So I think this can be closed?

@thedrow
Copy link
Contributor

thedrow commented Feb 12, 2014

@clee704 I don't believe so. @jamesls is right. FakeRedis should pass an isinstance check for redis.Redis. FakeRedis is a fake so it should behave exactly like the real Redis object when possible.

@clee704
Copy link
Contributor Author

clee704 commented Apr 24, 2014

I added a commit that makes FakeRedis inherit Redis.

Some libraries check whether the type of an argument is
redis.StrictRedis or not. If FakeStrictRedis is complete, this
inheritance should make no difference. It also makes FakeRedis
inherit redis.Redis
@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 79fa363 on clee704:inherit-redis into 9ed4351 on jamesls:master.

@clee704
Copy link
Contributor Author

clee704 commented Aug 4, 2014

rebased to the current master

@btashton
Copy link
Contributor

btashton commented Mar 4, 2015

This would also allow for directly using some of the convenience functions in StrictRedis. Right now in #73 I have to copy over the iterator code because I cannot do something like:

def scan_iter(self, match=None, count=None):
    return redis.StrictRedis.scan_iter(self, match, count)

@btashton
Copy link
Contributor

btashton commented Mar 4, 2015

On second though this seems like it might not be a great idea because you then have to add NotImplementedError on all of the not supported library functions.

@jamesls
Copy link
Owner

jamesls commented Mar 20, 2015

After thinking about it more, I tend to agree with @btashton. Subclassing would mean we may inadvertently call into the real redis-py module, which is not ideal. Given it's easy to make your own custom subclass that inherits from both classes, I'd prefer not to have this in fakeredis.

Closing for now.

@jamesls jamesls closed this Mar 20, 2015
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.

None yet

5 participants