Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1665)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 years, 5 months ago by Antoine Pitrou
Modified:
8 years, 5 months ago
Reviewers:
nadeem vawda <nadeem.vawda
CC:
bugs_python.org
Visibility:
Public.

Patch Set 1 #

Total comments: 9
Unified diffs Side-by-side diffs Delta from patch set Stats (+913 lines, -2075 lines) Patch
A Lib/bz2.py View 1 chunk +487 lines, -0 lines 2 comments Download
M Lib/test/test_bz2.py View 6 chunks +102 lines, -20 lines 0 comments Download
M Modules/_bz2module.c View 3 chunks +322 lines, -2053 lines 6 comments Download
M setup.py View 1 chunk +2 lines, -2 lines 1 comment Download

Messages

Total messages: 1
Antoine Pitrou
8 years, 5 months ago (2011-03-13 17:31:28 UTC) #1
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b