[issue5863] bz2.BZ2File should accept other file-like objects. - Code Review (original) (raw)

http://codereview.appspot.com/4274045/diff/1/Lib/bz2.py File Lib/bz2.py (right):

http://codereview.appspot.com/4274045/diff/1/Lib/bz2.py#newcode25 Lib/bz2.py:25: class BZ2File: Is there any reason it doesn't inherit io.BufferedIOBase? (it should also bring you a couple of methods implemented for free: readlines, writelines, iter, next, enter, exit)

You should probably also implement fileno() (simply return self.fp.fileno()) and the closed property.

http://codereview.appspot.com/4274045/diff/1/Lib/bz2.py#newcode386 Lib/bz2.py:386: class BZ2Compressor: I don't think there's a point in a Python wrapper, since the wrapper is so trivial. Just do the lock operations in C. Same for BZ2Decompressor.

http://codereview.appspot.com/4274045/diff/1/Modules/_bz2module.c File Modules/_bz2module.c (left):

http://codereview.appspot.com/4274045/diff/1/Modules/_bz2module.c#oldcode123 Modules/_bz2module.c:123: #ifdef WITH_THREAD As mentioned in Lib/bz2.py, I would keep the lock on the C side since it isn't significantly more complicated, and it avoids having to write a Python wrapper around the compressor and decompressor types.

http://codereview.appspot.com/4274045/diff/1/Modules/_bz2module.c File Modules/_bz2module.c (right):

http://codereview.appspot.com/4274045/diff/1/Modules/_bz2module.c#newcode3 Modules/_bz2module.c:3: #include "Python.h" Since this is a new start, perhaps we should add #define PY_SSIZE_T_CLEAN before including "Python.h"? This will ensure no code in the module will rely on the old behaviour.

http://codereview.appspot.com/4274045/diff/1/Modules/_bz2module.c#newcode48 Modules/_bz2module.c:48: "libbzip2 was not compiled correctly"); Just a nit, but I'm not sure there's any point in renaming "the bz2 library" to "libbzip2"? (also, under Windows I'm not sure the library naming convention is the same as under Unix)

http://codereview.appspot.com/4274045/diff/1/Modules/_bz2module.c#newcode78 Modules/_bz2module.c:78: "Unrecognized error from libbzip2: %d", bzerror); Out of curiousity, did you encounter this condition?

http://codereview.appspot.com/4274045/diff/1/Modules/_bz2module.c#newcode122 Modules/_bz2module.c:122: c->bzs.avail_out = PyBytes_GET_SIZE(result); Do note that avail_in and avail_out are 32-bit ints, and therefore this is not 64-bit clean. I guess you're just copying the old code here, but that would deserve a separate patch later. Perhaps add a comment in the meantime.

http://codereview.appspot.com/4274045/diff/1/Modules/_bz2module.c#newcode209 Modules/_bz2module.c:209: "Provide a block of data to the compressor."}, You could instead re-use the old, more precise docstrings. Also, using PyDoc_STRVAR is preferred so as to make it easier to modify multi-line docstrings.

http://codereview.appspot.com/4274045/diff/1/setup.py File setup.py (right):

http://codereview.appspot.com/4274045/diff/1/setup.py#newcode1236 setup.py:1236: exts.append( Extension('_bz2', ['_bz2module.c'], The Windows build files probably need updating as well. Can you do it? Otherwise I'll have a try.