fixes issues #44 #45 setDecoder not working, TypeError when reJSON returns None #46
Conversation
bentsku
changed the title
|
Kudos, SonarCloud Quality Gate passed!
|
rejson/client.py
Outdated
| def _decode(self, s, *args, **kwargs): | ||
| try: | ||
| return self._decoder.decode(s, *args, **kwargs) | ||
| except TypeError: | ||
| if s is not None: | ||
| raise | ||
| return None |
There was a problem hiding this comment.
| def _decode(self, s, *args, **kwargs): | |
| try: | |
| return self._decoder.decode(s, *args, **kwargs) | |
| except TypeError: | |
| if s is not None: | |
| raise | |
| return None | |
| def _decode(self, s, *args, **kwargs): | |
| try: | |
| return self._decoder.decode(s, *args, **kwargs) if s is not None else None | |
| except TypeError: | |
| raise |
There was a problem hiding this comment.
If None always raises a TypeError in decode, can we skip the attempt to decode it?
There was a problem hiding this comment.
I guess we can, I did this to not change the behaviour of the decoder, in the case the user uses a custom decoder and wanted to return a custom value. With this, we override the implementation of the decoder.
But in the general case, this is much better, as we don't even need the try...except clause. If that is okay to override the decoder implementation, then I prefer it too.
There was a problem hiding this comment.
The default decoder would skip attempting to decode None.
If, for some reason, users would like to handle None in the decoder, they can use their own decoder with setDecoder, so all options are still available.
There was a problem hiding this comment.
If they use their own decoder, it would still call the function _decode, and if the query result was None, would not attempt to use it, and return None before. This function is called before the call to the default or custom decoder decode function. But it only prevents users to handle None, which can be fine, they can handle it later on.
There was a problem hiding this comment.
Right, what I should have written is they could still use set_response_callback, e.g.,
set_response_callback('json.get', self.myDecoderMethod)
(but that should be done per command)
And then they can still do their own decoding, if they really insist,
But it could also simply be handled later on as you suggested (after the default decoding)
Since this PR is also fixing setDecoder, do you have a test case also for it?
There was a problem hiding this comment.
Perfect, yes if there are very specific user needs, it can always be done.
I believe I often code with the EAFP python way, but the performance in case of a None value of your solution are 10x better, and about the same if the value isn't None.
I just pushed the test for setDecoder and also one more operation on the pipeline test (issue #44). Thanks again for the review!
There was a problem hiding this comment.
@bentsku LGTM. Thank you for your contribution and for a fruitful discussion!
|
Kudos, SonarCloud Quality Gate passed!
|
|
Kudos, SonarCloud Quality Gate passed!
|
One possibility to fix both #44 and #45. I am not 100% certain on the ramifications, though I doubt it would create any problem in a normal usage of rejson-py. I've tested locally with the
jsonmodule, andujson.