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

#6715: xz compressor support

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 years, 6 months ago by devurandom
Modified:
7 years, 5 months ago
Reviewers:
victor.stinner, merwok, nadeem.vawda
CC:
loewis, barry, Georg, doko, jcea, amaury.forgeotdarc, arekm_maven.pl, lars.gustaebel, AntoinePitrou, haypo, nadeem.vawda, ned.deily, nicdumz_gmail.com, eric.araujo, Christophe Simonis, rcoyner_gmail.com, peroyvind_mandriva.org, asvetlov, Nikratio, python_lost.co.nz, garen.parham_gmail.com, ysj.ray, tdjacr.foss_gmail.com, ockham_raz.or.at, p.j.a.cock_googlemail.com, strombrg_gmail.com, shirishag75_gmail.com, tshepang, devnull_psf.upfronthosting.co.za, jeremy_jeremybanks.ca, bitsink_gmail.com
Visibility:
Public.

Patch Set 1 #

Patch Set 2 #

Total comments: 5

Patch Set 3 #

Total comments: 3

Patch Set 4 #

Total comments: 7

Patch Set 5 #

Total comments: 4

Patch Set 6 #

Total comments: 10

Patch Set 7 #

Total comments: 37

Patch Set 8 #

Patch Set 9 #

Unified diffs Side-by-side diffs Delta from patch set Stats Patch
Doc/library/archiving.rst View 1 chunk +2 lines, -1 line 0 comments Download
Doc/library/bz2.rst View 3 4 5 6 7 8 1 chunk +0 lines, -3 lines 0 comments Download
Doc/library/gzip.rst View 3 4 5 6 7 8 1 chunk +0 lines, -3 lines 0 comments Download
Doc/library/lzma.rst View 3 4 5 6 7 8 1 chunk +0 lines, -3 lines 0 comments Download
Doc/library/shutil.rst View 5 chunks +7 lines, -3 lines 0 comments Download
Doc/library/tarfile.rst View 1 chunk +2 lines, -1 line 0 comments Download
Doc/library/zipfile.rst View 3 4 5 6 7 8 1 chunk +0 lines, -3 lines 0 comments Download
Doc/library/zlib.rst View 3 4 5 6 7 8 1 chunk +1 line, -3 lines 0 comments Download

Messages

Total messages: 17
ezio.melotti
http://bugs.python.org/review/6715/diff/3315/10322 File Lib/lzma.py (right): http://bugs.python.org/review/6715/diff/3315/10322#newcode55 Lib/lzma.py:55: mode can br 'r' for reading (default) or 'w' ...
7 years, 8 months ago #1
amaury.forgeotdarc
http://bugs.python.org/review/6715/diff/3327/10361 File Lib/lzma.py (right): http://bugs.python.org/review/6715/diff/3327/10361#newcode55 Lib/lzma.py:55: mode can br 'r' for reading (default) or 'w' ...
7 years, 8 months ago #2
nadeem.vawda
On 2011/09/13 03:51:48, ezio.melotti wrote: > http://bugs.python.org/review/6715/diff/3315/10322#newcode55 > Lib/lzma.py:55: mode can br 'r' for reading ...
7 years, 8 months ago #3
nadeem.vawda
On 2011/09/15 09:03:12, amaury.forgeotdarc wrote: > http://bugs.python.org/review/6715/diff/3327/10361#newcode55 > Lib/lzma.py:55: mode can br 'r' for reading ...
7 years, 8 months ago #4
amaury.forgeotdarc
> > Did you consider using PyArg_ParseTupleAndKeywords? > > I did consider that initially, but ...
7 years, 8 months ago #5
nadeem.vawda
On 2011/09/15 10:35:23, amaury.forgeotdarc wrote: > So, extract "preset" first, then use PyArg_ParseTupleAndKeywords > for ...
7 years, 8 months ago #6
eric.araujo
Great stuff. Some suggestions for doc edits below. http://bugs.python.org/review/6715/diff/3328/10368 File Doc/library/lzma.rst (right): http://bugs.python.org/review/6715/diff/3328/10368#newcode5 Doc/library/lzma.rst:5: :synopsis: ...
7 years, 8 months ago #7
nadeem.vawda
On 2011/09/15 10:35:23, amaury.forgeotdarc wrote: > Maybe something like: > > preset_obj = PyMapping_GetItemString(spec, "preset"); ...
7 years, 8 months ago #8
nadeem.vawda
On 2011/09/15 18:00:06, eric.araujo wrote: > http://bugs.python.org/review/6715/diff/3328/10368#newcode98 > Doc/library/lzma.rst:98: This is the default (and the ...
7 years, 8 months ago #9
amaury.forgeotdarc
Nice! http://bugs.python.org/review/6715/diff/3335/10393 File Modules/_lzmamodule.c (right): http://bugs.python.org/review/6715/diff/3335/10393#newcode142 Modules/_lzmamodule.c:142: "Value too large for " #TYPE "type"); \ ...
7 years, 8 months ago #10
nadeem.vawda
On 2011/09/16 01:18:15, amaury.forgeotdarc wrote: > Nice! Thanks :) > http://bugs.python.org/review/6715/diff/3335/10393#newcode142 > Modules/_lzmamodule.c:142: "Value too ...
7 years, 8 months ago #11
loewis
http://bugs.python.org/review/6715/diff/3335/10393 File Modules/_lzmamodule.c (right): http://bugs.python.org/review/6715/diff/3335/10393#newcode115 Modules/_lzmamodule.c:115: return _PyBytes_Resize(buf, size + BIGCHUNK); This has quadratic performance. ...
7 years, 7 months ago #12
AntoinePitrou
Here are a couple of comments. I haven't take a very detailed look, but it ...
7 years, 7 months ago #13
nadeem.vawda
> Here are a couple of comments. I haven't take a very detailed look, but ...
7 years, 7 months ago #14
victor.stinner_haypocalc.com
Here is a quick review. Sorry, I didn't read the issue. http://bugs.python.org/review/6715/diff/3506/10975 File Doc/library/lzma.rst (right): ...
7 years, 6 months ago #15
eric.araujo
http://bugs.python.org/review/6715/diff/3506/10975 File Doc/library/lzma.rst (right): http://bugs.python.org/review/6715/diff/3506/10975#newcode59 Doc/library/lzma.rst:59: Iteration and the :keyword:`with` statement are supported. “Context management” ...
7 years, 6 months ago #16
nadeem.vawda
7 years, 6 months ago #17
http://bugs.python.org/review/6715/diff/3506/10975
File Doc/library/lzma.rst (right):

http://bugs.python.org/review/6715/diff/3506/10975#newcode21
Doc/library/lzma.rst:21: module. However, note that :class:`LZMAFile` is *not*
thread-safe, unlike
On 2011/11/17 02:47:54, haypo wrote:
> Is it possible to compress two files at the same on two cpus? Or the module
uses
> the GIL? (What do you mean by thread safety?)

I've updated the docs to clarify. Basically, because LZMAFile
is implemented in Python (and not C), concurrent reads or writes
on a file can leave the cache and position information in an
inconsistent state.

http://bugs.python.org/review/6715/diff/3506/10975#newcode39
Doc/library/lzma.rst:39: *fileobj*), or operate directly on a named file (named
by *filename*).
On 2011/11/17 02:47:54, haypo wrote:
> You may mention that the fileobj is not closed when the LZMAFile object is
> destroyed or by LZMAFile.__exit__.

Done.

http://bugs.python.org/review/6715/diff/3506/10975#newcode58
Doc/library/lzma.rst:58: :class:`io.BufferedIOBase`, except for :meth:`detach`
and :meth:`truncate`.
On 2011/11/17 02:47:54, haypo wrote:
> "except for detach and truncate" is not clear: I understand that methods are
not
> defined. These methods are defined but not supported (raise
> io.UnsupportedOperation which is correct).

I've replaced "provides" with "supports".

http://bugs.python.org/review/6715/diff/3506/10975#newcode59
Doc/library/lzma.rst:59: Iteration and the :keyword:`with` statement are
supported.
On 2011/11/17 02:47:54, haypo wrote:
> don't we have a name for the with keyword? The context manager protocol?

Saying "the context manager protocol" looks nicer, but I agree
with Éric that way it is now is more newbie-friendly.

http://bugs.python.org/review/6715/diff/3506/10975#newcode98
Doc/library/lzma.rst:98: data has not been corrupted. Possible values are:
On 2011/11/17 02:47:54, haypo wrote:
> What happens if the check is not supported? An exception is raised?

Done.

http://bugs.python.org/review/6715/diff/3506/10975#newcode116
Doc/library/lzma.rst:116: ``9`` (inclusive), optionally OR-ed with the constant
On 2011/11/17 02:47:54, haypo wrote:
> Which value is the fastest, 0 or 9? (Which value produces the smallest
output?)

Done.

http://bugs.python.org/review/6715/diff/3506/10975#newcode136
Doc/library/lzma.rst:136: The compressor may not be used after this method has
been called.
On 2011/11/18 11:40:37, eric.araujo wrote:
> IIUC here “may not” means “cannot”.

Correct. "Cannot" seems clearer, though, so I've changed it.

http://bugs.python.org/review/6715/diff/3506/10975#newcode291
Doc/library/lzma.rst:291: Reading in a compressed file::
On 2011/11/17 02:47:54, haypo wrote:
> A ":" is missing in the output. You should use ": ::" or write "::" on a
> newline.

Are you sure? I get a ":" in the output when I generate the docs.

http://bugs.python.org/review/6715/diff/3506/10975#newcode294
Doc/library/lzma.rst:294: with lzma.LZMAFile("/home/alyssa/file.xz") as f:
On 2011/11/17 02:47:54, haypo wrote:
> Why do you use an absolute path? "file.xz" should be enough for an example.

Done.

http://bugs.python.org/review/6715/diff/3506/10978
File Lib/lzma.py (right):

http://bugs.python.org/review/6715/diff/3506/10978#newcode175
Lib/lzma.py:175: return self._mode == _MODE_WRITE
On 2011/11/17 02:47:54, haypo wrote:
> A closed file is no more writable? Hum. TextIOWrapper.writable() raises
> ValueError('I/O operation on closed file') if the file is closed. I don't know
> what is the best choice for this class.

I think it makes sense to raise a ValueError in this case, and
the changes needed to do this are much smaller than I expected.
Done.

As an aside, I think this change should also be made to BZ2File
(which I based LZMAFile off of). What do you think?

http://bugs.python.org/review/6715/diff/3506/10978#newcode372
Lib/lzma.py:372: optional arguments *format*, *check*, *preset* and *filters*.
On 2011/11/17 02:47:54, haypo wrote:
> reST syntax is not supported in docstring, so *bold* should be avoided.

Éric is correct - I was not trying to make the argument names
render in bold, but rather simply emphasizing that they are
arguments. I would find the sentence ambiguous if they were not
emphasized like this, and I didn't want to have to rephrase
the sentence in a more roundabout way.

http://bugs.python.org/review/6715/diff/3506/10980
File Modules/_lzmamodule.c (right):

http://bugs.python.org/review/6715/diff/3506/10980#newcode150
Modules/_lzmamodule.c:150: if (PyErr_Occurred()) \
On 2011/11/17 02:47:54, haypo wrote:
> You can first check if val == (unsigned long)-1

Is there any point in doing so? (unsigned long)-1 is a valid
return value for PyLong_AsUnsignedLong, so the code would need
to check for an exception directly anyway.

http://bugs.python.org/review/6715/diff/3506/10980#newcode183
Modules/_lzmamodule.c:183: preset_obj = PyMapping_GetItemString(spec, "preset");
On 2011/11/17 02:47:54, haypo wrote:
> You may check PyMapping_Check(spec) here

Again, is there any point in doing this? PyMapping_GetItemString
raises an exception if spec is not a mapping, so the extra
check seems redundant.

http://bugs.python.org/review/6715/diff/3506/10980#newcode292
Modules/_lzmamodule.c:292: if (PyErr_Occurred())
On 2011/11/17 02:47:54, haypo wrote:
> You can first check if f->id == (unsigned Py_LONG_LONG)-1

See my reply for INT_TYPE_CONVERTER_FUNC()

http://bugs.python.org/review/6715/diff/3506/10980#newcode436
Modules/_lzmamodule.c:436: "The compressor object may not be used after this
method is called.\n");
On 2011/11/17 02:47:54, haypo wrote:
> may not => cannot

Done.

http://bugs.python.org/review/6715/diff/3506/10980#newcode1001
Modules/_lzmamodule.c:1001: if (lzma_check_is_supported(check_id))
On 2011/11/17 02:47:54, haypo wrote:
> You can use PyBool_FromLong

Done.
Sign in to reply to this message.

RSS Feeds Recent Issues | This issue
This is Rietveld 894c83f36cb7+