protocol/display: fix Python 3 buffer abstraction#62
Conversation
Current coverage is 81.24% (diff: 100%)@@ master #62 diff @@
==========================================
Files 39 39
Lines 4399 4415 +16
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 3570 3587 +17
+ Misses 829 828 -1
Partials 0 0
|
vasily-v-ryabov
left a comment
vasily-v-ryabov
left a comment
There was a problem hiding this comment.
Good observation. Just let me criticize some minor things.
|
|
||
| class bytesview(object): | ||
|
|
||
| def __init__(self, data, offset=None, size=None): |
There was a problem hiding this comment.
Why not just offset=0 by default? It saves 2 lines below.
| if isinstance(data, bytes): | ||
| view = memoryview(data) | ||
| elif isinstance(data, bytesview): | ||
| view = data._view |
There was a problem hiding this comment.
All static analyzers will produce a warning for this line. Accessing pseudo-private member of another object can be eliminated with a read-only property view for the _view field (as an option). Or you may make it public for simplicity.
| elif isinstance(data, bytesview): | ||
| view = data._view | ||
| else: | ||
| raise ValueError('unsupported type: %s' % type(data)) |
There was a problem hiding this comment.
Maybe raise TypeError('unsupported type: {}'.format(type(data))) is better and should work on py2.6+.
There was a problem hiding this comment.
OK, but we don't support 2.6 anymore. In fact, there's a extraneous view = memoryview(data) in the Python 2 code that made me realize that memoryview is also supported in Python 2, starting with 2.7, so we could further simplify the code by just using one BytesView class for all supported Python versions. What do you think?
There was a problem hiding this comment.
I remember, we don't support py2.6 (just mentioned it because .format() is new in 2.6). I like the idea of BytesView class.
|
|
||
| def bytesview(data, offset=None, size=None): | ||
| if not isinstance(data, (bytes, buffer)): | ||
| raise ValueError('unsupported type: %s' % type(data)) |
There was a problem hiding this comment.
The same as above.
|
Thanks for the review, I'll amend. |
With Python 2, slicing a buffer yield new data, but with Python 3, slicing a memoryview return another memoryview: > python2 -c 'print buffer(b"12345")[1:3].decode()' 23 > python3 -c 'print(memoryview(b"12345")[1:3].decode())' Traceback (most recent call last): File "<string>", line 1, in <module> AttributeError: 'memoryview' object has no attribute 'decode'
benoit-pierre
force-pushed
the
fix_python3_buffer_abstraction
branch
from
69768a3 to
95ba513
Compare
|
Unfortunately, > python2 -c 'print bytes(memoryview(b"12345"))'
<memory at 0x2ae8f7a2f9d0>
> python3 -c 'print(bytes(memoryview(b"12345")))'
b'12345'So I kept the 2 different code paths for Python 2 and 3, and addressed your comments. |
3 participants
With Python 2, slicing a buffer yield new data, but with Python 3, slicing a memoryview return another memoryview: