Added the ability (and tests of) integer, float, tuple, and frozenset keys #74
Conversation
|
Thanks for the PR, I have 2 main questions:
|
|
Performance should be trivially affected. For string/bytes keys (all that currently work) the only overhead is a few conditionals. For the new key types, there is the slight overhead of As for other types, I do not have a good answer. While there are a few exceptions, any hashable python object can be a key but would require manual implementation to handle them. One option would be to instead pickle the object and compare the keys that way. I think this introduces even more overhead. Also, should the user wish to look at the sqlite file elsewhere, the keys would be imperceptible (In my use of I will address a few more issues in the discussion of #73 |
There was a problem hiding this comment.
Please run your code through flake8 so that it conforms to the PEP8 standard. e.g. 2 line spacing after functions, no lines with only whitespace, etc.
Next, while I think your new behavior is useful, it introduces the risk of backward compatibility and performance issues. So, I think this behavior should be disabled by default. Add two keyword parameters to the constructor:
- serialize: convert an arbitary object to a string
- deserialize: convert an arbitrary string to an object
where deserialize(serialize(obj)) == obj
By default, both of them should be None, in which case, the old behavior is used. If the user specifies only one of them, raise ValueError. If the user specifies both of them, then we can use them instead of your current convertkey/unconvertkey functions.
This approach has several benefits:
- Maintains existing behavior (compatibility, efficiency)
- Addresses the problem in the original issue
- Extensible: people can use their own serialization functions if the built-in ones are insufficient
| @@ -213,7 +214,7 @@ def __bool__(self): | |||
| def iterkeys(self): | |||
| GET_KEYS = 'SELECT key FROM "%s" ORDER BY rowid' % self.tablename | |||
| for key in self.conn.select(GET_KEYS): | |||
| yield key[0] | |||
| yield _unconvertkey(key[0]) | |||
There was a problem hiding this comment.
Please use a more helpful function name, like serialize (and conversely, deserialize).
|
Ping @Jwink3101 are you able to finish this PR? |
| return '___.float.' + repr(key) | ||
| # These are recursive | ||
| elif isinstance(key,tuple): | ||
| return '___.tuple.' + json.dumps([_convertkey(k) for k in key]) |
There was a problem hiding this comment.
It would be very helpful to allow passing in a JSONEncoder to be used here as a cls=.. argument, to allow inclusion of types which need help serialising.
This could be done after your PR, if you like, but that may be a useful element in the design of this PR.
| db['1'] = 1 | ||
| db[1] = 'ONE' | ||
| db[('a',1)] = 'testtuple' | ||
| db[frozenset([1,2,'2'])] = 'testfrozenset' |
There was a problem hiding this comment.
this should have assertions about the underlying generated key names, possibly as separate test method, but also here so it is possible to understand the implementation by reading the tests
|
This might be part of a more significant issue, a user treating an Currently, keys are mutated: This seems to occur because the default |
|
The problem with using python’s My code is one option but another is just education. |
closes #73