-
Notifications
You must be signed in to change notification settings - Fork 275
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
add ensure_binary/str/text helper functions #204
Conversation
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.
Thanks for the PR. A few things:
documentation/index.rst
needs to be updated.- There need to be tests for the second and third parameters to each function.
- The tests should actually check the type of what they're getting back from the functions under test, since converting between types is the main point of these functions.
six.py
Outdated
|
||
|
||
def ensure_str(s, encoding='utf-8', errors='strict'): | ||
""" A helper function to ensure output is six.str_type. |
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.
six.str_type
doesn't exist.
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.
good catch, will update the doc to six.string_types and rename function to ensure_string
six.py
Outdated
raise TypeError("not expecting type '%s'" % type(s)) | ||
if isinstance(s, text_type): | ||
return s.encode(encoding, errors) | ||
else: |
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.
This can be elif isinstance(s, binary_type):
and then the first if
statement in this function may be removed.
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.
@benjaminp do you mean something like?
def ensure_binary(s, encoding='utf-8', errors='strict'):
if isinstance(s, text_type):
return s.encode(encoding, errors)
elif isinstance(s, binary_type):
return s
else:
raise TypeError("not expecting type '%s'" % type(s))
I'd like to keep the raise TypeError
part to fail the error loudly; be consistent with the raise
behavior from ensure_string
.
six.py
Outdated
raise TypeError("not expecting type '%s'" % type(s)) | ||
if isinstance(s, binary_type): | ||
return s.decode(encoding, errors) | ||
else: |
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.
The isinstance
checks can be rewritten in a way similar to what I suggested in ensure_binary
.
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.
sure, will update to the following if we agree to keep raise exception
part
if isinstance(s, binary_type):
return s.decode(encoding, errors)
elif isinstance(s, text_type):
return s
else:
raise TypeError("not expecting type '%s'" % type(s))
Thanks for the review, @benjaminp
|
- add checking for type in tests; - fix six.str_types typo; - simplify logic in ensure_;
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.
Almost there. There still could be some more testing of errors
and encoding
. e.g., what happens when invalid utf-8 is given with error="strict|replace|igonre"
?
documentation/index.rst
Outdated
.. function:: ensure_binary(s, encoding='utf-8', errors='strict') | ||
|
||
A helper function to ensure output is :data:`binary_type`. ``encoding``, ``errors`` | ||
are the same as https://docs.python.org/3/library/stdtypes.html#str.encode |
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.
Write
:meth:`py3:str.encode`
here and below
test_six.py
Outdated
@@ -903,3 +903,56 @@ def __bytes__(self): | |||
assert str(my_test) == six.u("hello") | |||
|
|||
assert getattr(six.moves.builtins, 'bytes', str)(my_test) == six.b("hello") | |||
|
|||
|
|||
class EnsureTests(unittest.TestCase): |
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.
Don't need to inherit from TestCase
here.
Hi! @benjaminp, @jz1371 is it moving forward? @jz1371 if you have no time I am happy to finish it. You can see there is a need for these functions in the community. They even made it to a keynote at PyCon 🤣 https://youtu.be/66XoCk79kjM?t=31m15s |
@benjaminp, any other changes you'd like to have in this PR? |
THIS IS SO AWESOME!! I was watching the Pycon 2017 Instagram video and thinking "This needs to be in six!" and it is already. Any sense on when this will be in a release? |
Indeed. I am looking forward to remove this from m codebase :) |
python-autopxd already requires six, but the new `ensure_binary` function is only in the git repo and hasn't been released to PyPI yet, so I temporarily copied it here. The source comes from: benjaminp/six#204 It's a trivial function (every py2.py3 project has its own way to do the same thing). Fixes tarruda#12 Supersedes tarruda#3
It would be great if we could have this released to pypi some time soon, thanks! |
@benjaminp Hello there! Any idea when this may be released? |
six==1.14 will work with the error 'module 'six' has no attribute 'ensure_str'' |
six==1.14.0 (with the .0) as in https://pypi.org/project/six/
|
Summary
add ensure_binary, ensure_str, ensure_text helper functions to solve the encoding compatibility
between py2 and py3 more easily.
-- ensure_binary, input --> six.binary_type
-- ensure_str, input -> six.string_type
-- ensure_text, input -> six.text_type
This is mentioned in Lisa Guo's keynote in Pycon 2017 (from 31:21 in https://www.youtube.com/watch?v=66XoCk79kjM), which helped Instagram to upgrade to Python 3 and I hope this can help other developers, too
Test Plan
add new unit tests to cover added code