python 2.7 Unicode compatible, with test#107
python 2.7 Unicode compatible, with test#107niconoe wants to merge 14 commits intolinsomniac:masterfrom niconoe:separate_commits
Conversation
|
|
Thanks, but why it failed checks? |
|
|
||
|
|
||
| # vim: ts=4 sw=4 et : | ||
| # vim: ts=4 sw=4 et : No newline at end of file |
There was a problem hiding this comment.
this is missing a new line which makes it an invalid file in POSIX, that's why it failed travis on python 2.6
There was a problem hiding this comment.
Nice catch, I was confused!
|
|
||
|
|
||
| # vim: ts=4 sw=4 et : | ||
| # vim: ts=4 sw=4 et : No newline at end of file |
There was a problem hiding this comment.
revert the newline removal
| @@ -1,3 +1,5 @@ | |||
| # -*- coding: utf-8 -*- | |||
|
|
|||
| val = buf.decode('utf8') | ||
| else: | ||
| # Bare bytes | ||
| val = buf |
| def test_unicode_value(self): | ||
| key = 'key' | ||
| value = six.u('Iñtërnâtiônàlizætiøn2') | ||
|
|
There was a problem hiding this comment.
I'd omit the blank line.
|
Does it also fix #80? |
|
|
||
| def test_binary_string(self): | ||
| # Binary strings should be cacheable | ||
| from zlib import compress, decompress |
There was a problem hiding this comment.
I'd import zlib at the top of the file and use zlib.compress(), etc.
| self.assertEqual(value, cached_value) | ||
|
|
||
| def test_binary_string(self): | ||
| # Binary strings should be cacheable |
There was a problem hiding this comment.
I guess this isn't accomplishing much.
| self.assertEqual(compressed_value, compressed_result) | ||
| self.assertEqual(value, decompress(compressed_result).decode()) | ||
|
|
||
| # Test set_many |
There was a problem hiding this comment.
The comments don't seem to add value (reviewing my own code). ;-)
| self.assertEqual(value, decompress(compressed_result).decode()) | ||
|
|
||
| # Test add | ||
| self.mc.add('binary1-add', compressed_value) |
There was a problem hiding this comment.
Now that I see the fix, I wonder if the 3 variants are needed if they all use the same encoding process? For example, we don't have 3 variants for test_unicode_value.
There was a problem hiding this comment.
Isn't that more future-proof to ensure it works with the 3 methods. In that case, I'll complete test_unicode_value() instead.
| # Bare bytes | ||
| val = buf | ||
| elif flags & Client._FLAG_TEXT: | ||
| val = buf.decode('utf8') |
There was a problem hiding this comment.
whilst you're at it you could make this utf-8 to be consistent with line 980
|
It's unclear to me if any of the |
| if isinstance(key, tuple): | ||
| if isinstance(key[1], six.text_type): | ||
| return (key[0], key[1].encode('utf8')) | ||
| return (key[0], key[1].encode('utf-8')) |
There was a problem hiding this comment.
Please don't make unrelated cleanups like that in a bug fix branch as it makes review more difficult, do it in a separate PR. (You've accidentally merged in the touch() fixes on this branch as well.
|
@niconoe, do you have time to update this and remove the unrelated changes? I'd rebase the branch and make the fix a single squashed commit. |
|
@timgraham like this? |
|
Updated and merged in #135, thanks! |
4 participants
To make life of the maintainer easier, I just grouped in this PR the fix for #79 by @dzhuang with the regression test by @tiumgraham.