add wrapper ducts, including redis #63
Conversation
| @@ -292,7 +292,7 @@ class name if not specified). | |||
|
|
|||
| atexit.register(self.disconnect) | |||
| self.__prepared = False | |||
| self.__getting = False | |||
There was a problem hiding this comment.
this is me nitting field naming. Not totally relevant for this PR.
There was a problem hiding this comment.
Hmmm... I think the original name makes more sense, but I'm probably biased (having written that code). This variable acts as an indicator for whether the Duct instance is currently preparing itself, and uses this information to prevent recursion during the preparation phase. It exists so that the code in _prepare can be written using normal self-attribute references, rather than using object.__getattribute__ everywhere. The new name '__getting' seems a little misleading.
There was a problem hiding this comment.
hold up - previously the name was __getting which I found strange since it's indicating that the Duct is preparing ... hence the rename to __preparing ... which way are you for?
There was a problem hiding this comment.
Haha... Right you are, good fellow. I'm sure there are some lessons in this: perhaps, don't do code review on your phone? Or perhaps not while looking after kids? In any case, I approve of this renaming :).
| logger.info('Disconnecting from Redis database ...') | ||
| self._redis_connection = None | ||
|
|
||
| @property |
There was a problem hiding this comment.
I'm thinking this should really be a class property but not sure how to make an abstract class property?
There was a problem hiding this comment.
Well... since there's no such thing as a class property (at least not in the standard library; you can simulate it using metaclasses), I'm not surprised that there is no abstract version of it ;).
|
Thanks for putting this together @danfrankj . This is indeed something along the lines of one of the approaches what we chatted about in #59; so thanks for giving it a shot. I gave it a bit more thought, and think we should probably just add a new database duct kind called 'nosql', and commit to some fairly common set of methods (get, set, find,... maybe others? We can always add more in the future). The only reason I have not already done this, although I have thought in this direction (thus #19), is that I don't actually have a whole lot of hands on experience with NoSQL databases, and am thus not sure which set of methods should actually be implemented. Is Doing it this way allows us to commit to a standard API (like our other duct types), including as part of that API a particular attribute which is the raw client here being wrapped (and as such, we wouldn't actually need to wrap it). This leads to a more consistent user experience, particularly when accessed via the DuctRegistry and its proxies. There may still be some utility to having a Happy to be convinced otherwise if you think this is not the right approach. What are your thoughts? |
Wrapper ducts to keep connection & configuration but forward commands to wrapped object