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

Remove the need for a custom __str__ method #7

Merged
merged 4 commits into from Oct 10, 2016

Conversation

Projects
None yet
3 participants
@c-w
Copy link
Contributor

commented Oct 10, 2016

Resolves #5

@coveralls

This comment has been minimized.

Copy link

commented Oct 10, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling e716157 on c-w:master into df190a5 on alexk307:master.

@alexk307
Copy link
Owner

left a comment

Great job! Just a few style comments, but it looks good :) Thank you for contributing.

@@ -0,0 +1,17 @@
class FakeRedisClient(object):

This comment has been minimized.

Copy link
@alexk307

alexk307 Oct 10, 2016

Owner

I'm not so sure that we need this when we can just mock the Redis Wrapper itself. See https://github.com/alexk307/redis_cache/blob/master/tests/test_redis_cache.py#L18 for example

This comment has been minimized.

Copy link
@c-w

c-w Oct 10, 2016

Author Contributor

Done.

argument on an object that does not implement a __str__ method
"""
redis_cache = RedisCache(self.address, self.port)
redis_cache.redis_client = FakeRedisClient()

This comment has been minimized.

Copy link
@alexk307

alexk307 Oct 10, 2016

Owner

instead of monkey patching yourself, you can just patch using the mock library :)

This comment has been minimized.

Copy link
@c-w

c-w Oct 10, 2016

Author Contributor

Done.


primitive_argument = 'cache hit test'

instance1 = TestClass()

This comment has been minimized.

Copy link
@alexk307

alexk307 Oct 10, 2016

Owner

Maybe we should consider putting this in a loop instead of just calling the class/method 3 times. This is sort of a data driven test, so I think it should possibly be treated as such

This comment has been minimized.

Copy link
@c-w

c-w Oct 10, 2016

Author Contributor

Found a way to make this test simpler and rationalized it with the other test -- less code is better!

state2_instance1 = TestClass(state2)
state2_instance2 = TestClass(state2)

value1 = state1_instance1.some_method()

This comment has been minimized.

Copy link
@alexk307

alexk307 Oct 10, 2016

Owner

Could you comment a bit here as to what you're testing? It does not seem obvious on first glance

This comment has been minimized.

Copy link
@c-w

c-w Oct 10, 2016

Author Contributor

Made this test a bit more explicit.


try:
instance_namespace = arg.__class__.__name__
instance_state = sorted((field, _argument_to_string(value))

This comment has been minimized.

Copy link
@alexk307

alexk307 Oct 10, 2016

Owner

So this generates a cache key based on the object's name and the instance variables that it contains? And it gets those instance variables recursively, so if the instance variables are objects with state themselves, they also get expressed?

This comment has been minimized.

Copy link
@c-w

c-w Oct 10, 2016

Author Contributor

Yes, that's correct. I re-structured the code and added some more comments to make this clearer.

c-w added some commits Oct 10, 2016

Clean up the tests for the new auto-__str__ code
- Rationalize the complex/simple argument tests to remove duplication.
- Add some more explanations of what's being tested.
Make _argument_to_string easier to understand
- Add some more comments about what's going on and why we need the
  complexity/recursion.
- Minimize the scope of the try/catch block.
@coveralls

This comment has been minimized.

Copy link

commented Oct 10, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 6c6984f on c-w:master into df190a5 on alexk307:master.

@alexk307
Copy link
Owner

left a comment

just a quick comment!

self.assertEqual(instance1_return1, argument)
self.assertEqual(instance1_return2, argument)
self.assertEqual(instance1_return3, argument)
self.assertEqual(instance.call_count[argument], 1)

This comment has been minimized.

Copy link
@alexk307

alexk307 Oct 10, 2016

Owner

That makes a lot more sense! Thank you

self.assertEqual(instance1_return3, argument)
self.assertEqual(instance.call_count[argument], 1)

# the cache should also be hit for calls to the same method on a new

This comment has been minimized.

Copy link
@alexk307

alexk307 Oct 10, 2016

Owner

beautiful

state2 = SimpleObject('some complex type state', 123)

def cache_get(cache_key):
if cache_key == '-5024234577788600450':

This comment has been minimized.

Copy link
@alexk307

alexk307 Oct 10, 2016

Owner

is there anyway that we can calculate these hash values instead of hardcoding them?

This comment has been minimized.

Copy link
@alexk307

alexk307 Oct 10, 2016

Owner

other than this LGTM!

This comment has been minimized.

Copy link
@alexk307

alexk307 Oct 10, 2016

Owner

also, FWIW I'm thinking of not doing these numerical hash values, but instead using the method names/signatures as the hash keys. This way I can implement some sort of cache invalidation/busting.

This comment has been minimized.

Copy link
@c-w

c-w Oct 10, 2016

Author Contributor

The cache key formatting is now a little bit more complicated so this required another refactor: rationalize the cache key formatting logic into one place so that it can be re-used from the unit tests.

Extract _generate_cache_key method
This rationalizes all the logic required to create a cache key into one
place and therewith lets us remove the hard-coded or copy/pasted key
formatting logic in the unit tests.
@coveralls

This comment has been minimized.

Copy link

commented Oct 10, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 01c64db on c-w:master into df190a5 on alexk307:master.

@ghost

This comment has been minimized.

Copy link

commented Oct 10, 2016

Lets give it a go!

@ghost

This comment has been minimized.

Copy link

commented Oct 10, 2016

Oops apparently my mobile thinks I'm on this account. I'll merge when I get home

@alexk307

This comment has been minimized.

Copy link
Owner

commented Oct 10, 2016

@c-w merged, thanks again :)

@alexk307 alexk307 merged commit 4b47661 into alexk307:master Oct 10, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 100.0%
Details
@alexk307

This comment has been minimized.

Copy link
Owner

commented Oct 10, 2016

@c-w if you have any projects that need contributing, let me know, I'd be glad to lend a hand as well. Cheers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.