[issue5863] bz2.BZ2File should accept other file-like objects.

[Antoine Pitrou, 2011-03-13 17:31:28]

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.