-
Notifications
You must be signed in to change notification settings - Fork 101
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
Allow "encoding" and "errors" attributes to be updated at runtime #96
Conversation
This patch provides three changes: 1. Adds the Reader.set_encoding method. This method accepts the "encoding" and "errors" arguments, verifies that both are either NULL or found within the codecs module and then updates the Reader's state to use the new values. The motivation here is to allow a user to update the Reader's encoding functionality, perhaps temporarily. redis-py plans to use this to provide context manager support. Something like: ```python client = redis.Redis(encoding="utf-8") client.get("my-utf8-key") with client.response_context(encoding=None) as no_decode_client: # gets the value of "my-bytes-key" and returns it as a bytes object no_decode_client.get("my-bytes-key") ``` In the above example, the context manager will call Reader.set_encoding, passing the encoding=None argument to update the Reader state. When the context manager exits, it will again call Reader.set_encoding with the original "encoding" and "errors" values. 2. Allows both Reader.__init__ and Reader.set_encoding to accept Python None values for "encoding" and "errors". When encoding=None is specified, hiredis-py will not decode strings and instead return bytes objects. 3. Passing an invalid "encoding" to Reader.__init__ or Reader.set_encoding will immediately raise a LookupError. Prior to this, Reader.__init__ would accept an invalid encoding and then raise the LookupError the first time the Reader attempted to decode a string. Raising the error immediately seems preferable.
Do we need to expose reading encoding, using tp_getset, tp_descr_get or some other methods? >>> print(reader.encoding)
'utf-8'
>>> reader.encoding = 'ascii'
>>> reader.encoding
'ascii' Overall looks good to me. |
I’m fine exposing read access to those attributes if you’d like, but it’s not something I need. I generally don’t like setting attributes that can raise exceptions, but that’s just a stylistic preference. I’ll do whatever you recommend. E.g, |
Let’s keep it this way for now, I’ll have another look at it |
src/reader.c
Outdated
if(_Reader_set_encoding(self, encoding, errors) == -1) | ||
return NULL; | ||
|
||
return Py_None; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is a reference count issue, better use Py_RETURN_NONE
@ifduyue Good catch! Made that change. |
Thanks, merged! |
This patch provides three changes:
Adds the
Reader.set_encoding
method. This method accepts the "encoding"and "errors" arguments, verifies that both are either NULL or found within
the codecs module and then updates the Reader's state to use the new values.
The motivation here is to allow a user to update the Reader's encoding
functionality at runtime, perhaps temporarily. redis-py plans to use this to provide
context manager support. Something like:
In the above example, the context manager will call
Reader.set_encoding
,passing the
encoding=None
argument to update the Reader state. When thecontext manager exits, it will again call
Reader.set_encoding
to restore the original "encoding" and "errors" values.Allows both
Reader.__init__
andReader.set_encoding
to accept Python Nonevalues for "encoding" and "errors". When
encoding=None
is specified,hiredis-py will not decode strings and instead return bytes objects.
Passing an invalid "encoding" to
Reader.__init__
orReader.set_encoding
will immediately raise a
LookupError
. Prior to this,Reader.__init__
wouldaccept an invalid encoding and then raise the
LookupError
the first timethe Reader attempted to decode a string. Raising the error immediately
seems preferable.