Fix support for callable coders #76
Conversation
| assert encoded_tuple == b'\x00' * 31 + b'\x01' + NULL_ENCODING | ||
| assert decode_single('(int,null)', encoded_tuple) == (1, None) | ||
|
|
||
| registry.unregister('null') |
There was a problem hiding this comment.
This unregister being necessary in the test is a hint to me that the API could be improved. The ideal I would look for is the pattern of initializing an object in a fixture, and then not worrying about state from one test polluting another test. That's awkward with this global registry singleton. Something like a make_registry() method that initializes a new registry with all the defaults would enables us to do:
@pytest.fixture
def registry():
return make_registry()Then we don't have to worry about unregistering.
Since there are other more pressing issues, maybe just capture the idea into an issue and we can come back to it later. (if you like it)
There was a problem hiding this comment.
I thought about making a fixture but just decided it was an okay hack for the time being. Also, I felt like it would be useful to test the functionality by calling encode_single and decode_single to make it more like an integration test instead of querying the registry directly and using the result of that. I'll make an issue for it.
There was a problem hiding this comment.
This should be wrapped in a try/finally, otherwise it's going to leave the registry polluted when this test fails.
There was a problem hiding this comment.
PR created for this: #79
| @@ -144,4 +144,4 @@ def from_type_str(cls, type_str, registry): # pragma: no cover | |||
| Used by ``ABIRegistry`` to get an appropriate encoder or decoder | |||
| instance for the given type string and type registry. | |||
| """ | |||
| raise NotImplementedError('Must implement `from_type_str`') | |||
| raise cls() | |||
There was a problem hiding this comment.
This line is confusing to me.
There was a problem hiding this comment.
Thank you for catching that. I intended for that to be return cls() and I believe my thinking was that it would make some of the examples in the registry documentation more simple but then I just went with slightly more complex examples. I'll make a PR to revert it right away.
There was a problem hiding this comment.
PR created for this: #78
| assert encoded_tuple == b'\x00' * 31 + b'\x01' + NULL_ENCODING | ||
| assert decode_single('(int,null)', encoded_tuple) == (1, None) | ||
|
|
||
| registry.unregister('null') |
There was a problem hiding this comment.
This should be wrapped in a try/finally, otherwise it's going to leave the registry polluted when this test fails.
* remove gitter, testing setup, and pandoc sections, add quotes to dev install
* remove gitter, testing setup, and pandoc sections, add quotes to dev install
What was wrong?
When adding support for tuple types and the
is_encodablefunction, support for simple callable coders was broken.How was it fixed?
Modified code in
is_encodableas well asencodinganddecodingmodules to handle simple callables. Added tests for functionality that wasn't working as expected.Cute Animal Picture