#6715: xz compressor support
|
Created:
2 years, 6 months ago by devurandom Modified:
1 year, 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 #
Created: 1 year, 5 months ago
MessagesTotal messages: 17
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' for writing. s/br/be/ http://bugs.python.org/review/6715/diff/3315/10323 File Lib/test/test_lzma.py (right): http://bugs.python.org/review/6715/diff/3315/10323#newcode39 Lib/test/test_lzma.py:39: format=lzma.FORMAT_ALONE, filters=FILTERS_RAW_1) Some of this might read better as context managers. http://bugs.python.org/review/6715/diff/3315/10323#newcode152 Lib/test/test_lzma.py:152: self.assertEqual(lzd.unused_data, b"") Can't you factor out the + self.assertFalse(lzd.eof) + out = lzd.decompress(XXX) + self.assertEqual(out, INPUT) + self.assertEqual(lzd.check, lzma.CHECK_YYY) + self.assertTrue(lzd.eof) + self.assertEqual(lzd.unused_data, b"") part in a specific assert method? (possibly with a few more argument with common defaults values, depending on what parts change) http://bugs.python.org/review/6715/diff/3315/10323#newcode396 Lib/test/test_lzma.py:396: """Remove a file, not raising an error if the file does not exist.""" Isn't there already something similar in test.support? (test.support.unlink maybe?) http://bugs.python.org/review/6715/diff/3315/10323#newcode991 Lib/test/test_lzma.py:991: INPUT = b""" Wasn't this in another test already? Maybe you can import it from there instead of copying it again.
Sign in to reply to this message.
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' for writing. 'a' is also allowed http://bugs.python.org/review/6715/diff/3327/10361#newcode145 Lib/lzma.py:145: finally: Should the file be closed even if compressor.flush() failed? io.TextIOWrapper.close() does not do it, for example. http://bugs.python.org/review/6715/diff/3327/10363 File Modules/_lzmamodule.c (right): http://bugs.python.org/review/6715/diff/3327/10363#newcode200 Modules/_lzmamodule.c:200: preset_obj = PyMapping_GetItemString(spec, "preset"); Did you consider using PyArg_ParseTupleAndKeywords?
Sign in to reply to this message.
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 (default) or 'w' for writing. > s/br/be/ Done. > http://bugs.python.org/review/6715/diff/3315/10323#newcode39 > Lib/test/test_lzma.py:39: format=lzma.FORMAT_ALONE, filters=FILTERS_RAW_1) > Some of this might read better as context managers. You're right; done. > http://bugs.python.org/review/6715/diff/3315/10323#newcode152 > Lib/test/test_lzma.py:152: self.assertEqual(lzd.unused_data, b"") > Can't you factor out the > + self.assertFalse(lzd.eof) > + out = lzd.decompress(XXX) > + self.assertEqual(out, INPUT) > + self.assertEqual(lzd.check, lzma.CHECK_YYY) > + self.assertTrue(lzd.eof) > + self.assertEqual(lzd.unused_data, b"") > part in a specific assert method? > (possibly with a few more argument with common defaults values, depending on > what parts change) Done. > http://bugs.python.org/review/6715/diff/3315/10323#newcode396 > Lib/test/test_lzma.py:396: """Remove a file, not raising an error if the file > does not exist.""" > Isn't there already something similar in test.support? (test.support.unlink > maybe?) Actually, I thought the same, but didn't find it in the docs and assumed I was mistaken. Turns out it just isn't documented. > http://bugs.python.org/review/6715/diff/3315/10323#newcode991 > Lib/test/test_lzma.py:991: INPUT = b""" > Wasn't this in another test already? > Maybe you can import it from there instead of copying it again. I'm not entirely comfortable with that; having tests depending on each other doesn't feel right to me. I'd rather have all the data for this test be together in one place, so that things won't break if someone unwittingly changes the data in test_zlib. That shouldn't ever happen, but still...
Sign in to reply to this message.
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 (default) or 'w' for writing. > 'a' is also allowed Done. > http://bugs.python.org/review/6715/diff/3327/10361#newcode145 > Lib/lzma.py:145: finally: > Should the file be closed even if compressor.flush() failed? > io.TextIOWrapper.close() does not do it, for example. Is there any benefit to leaving the file open? Note that self._closefp is only set to true if this is a file we opened in LZMAFile.__init__(). In this case, the user has no way of accessing the file object (except through self._fp, which is an undocumented implementation detail). The code never closes a file object passed in by the user, even if the call to decompressor.flush() succeeds. > http://bugs.python.org/review/6715/diff/3327/10363#newcode200 > Modules/_lzmamodule.c:200: preset_obj = PyMapping_GetItemString(spec, "preset"); > Did you consider using PyArg_ParseTupleAndKeywords? I did consider that initially, but in the end it works out to be worse. The problem is that the options struct needs to first be initialized based on 'preset', and after that we override only those fields that are explicitly specified. The only way I can think of to achieve this using PyArg_ParseTupleAndKeywords would be to parse the field values as PyObjects, and then explicitly check whether each one is NULL before converting it to a C integer and setting the corresponding field in the struct. This would be messier than the current way of doing things, since each field's assignment clause would need its own error handling.
Sign in to reply to this message.
> > Did you consider using PyArg_ParseTupleAndKeywords?
>
> I did consider that initially, but in the end it works out to be worse.
> The problem is that the options struct needs to first be initialized
> based on 'preset', and after that we override only those fields that
> are explicitly specified. The only way I can think of to achieve this
> using PyArg_ParseTupleAndKeywords would be to parse the field values as
> PyObjects, and then explicitly check whether each one is NULL before
> converting it to a C integer and setting the corresponding field in the
> struct. This would be messier than the current way of doing things,
> since each field's assignment clause would need its own error handling.
So, extract "preset" first, then use PyArg_ParseTupleAndKeywords
for other parameters? It's still better than all these calls to
PyUnicode_CompareWithASCIIString.
Maybe something like:
preset_obj = PyMapping_GetItemString(spec, "preset");
... parse preset ...
... allocate structure ...
... lzma_lzma_preset() ...
static char *optnames[] = {"preset", "dict_size", "lc", "lp", NULL};
PyArg_ParseTupleAndKeywords(
empty_tuple, spec,
"|OO&O&O&;invalid option for LZMA filter",
optnames,
&preset_obj,
uint32_converter, &options->dict_size,
uint32_converter, &options->lc,
uint32_converter, &options->lp);
Sign in to reply to this message.
On 2011/09/15 10:35:23, amaury.forgeotdarc wrote:
> So, extract "preset" first, then use PyArg_ParseTupleAndKeywords
> for other parameters? It's still better than all these calls to
> PyUnicode_CompareWithASCIIString.
>
> Maybe something like:
>
> preset_obj = PyMapping_GetItemString(spec, "preset");
> ... parse preset ...
> ... allocate structure ...
> ... lzma_lzma_preset() ...
> static char *optnames[] = {"preset", "dict_size", "lc", "lp", NULL};
> PyArg_ParseTupleAndKeywords(
> empty_tuple, spec,
> "|OO&O&O&;invalid option for LZMA filter",
> optnames,
> &preset_obj,
> uint32_converter, &options->dict_size,
> uint32_converter, &options->lc,
> uint32_converter, &options->lp);
I seem to recall that not working out for some reason, but I can't
quite remember why. I'll take another look at it this evening, though -
if it does work, it would certainly be cleaner than the current code.
Sign in to reply to this message.
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: A Python wrapper for the liblzma compression library. Take credit where credit is due! :) :moduleauthor: You :sectionauthor: You http://bugs.python.org/review/6715/diff/3328/10368#newcode98 Doc/library/lzma.rst:98: This is the default (and the only acceptable value) for I think you have one extra space in your indent here. http://bugs.python.org/review/6715/diff/3328/10368#newcode121 Doc/library/lzma.rst:121: Compress *data*, returning a :class:`bytes` object containing compressed What type should data be? http://bugs.python.org/review/6715/diff/3328/10368#newcode140 Doc/library/lzma.rst:140: For one-shot decompression, consider using :func:`decompress` instead. What’s the advantage of decompress? http://bugs.python.org/review/6715/diff/3328/10371 File Lib/lzma.py (right): http://bugs.python.org/review/6715/diff/3328/10371#newcode3 Lib/lzma.py:3: This module provides a file interface, classes for incremental I don’t understand “a file interface”. http://bugs.python.org/review/6715/diff/3328/10371#newcode68 Lib/lzma.py:68: When opening a file for reading, the 'preset' argument is not To distinguish string objects from parameter names, I tend to use 'string' and *param*. http://bugs.python.org/review/6715/diff/3328/10371#newcode111 Lib/lzma.py:111: elif mode in ("w", "a"): You can just use one string 'wa'.
Sign in to reply to this message.
On 2011/09/15 10:35:23, amaury.forgeotdarc wrote:
> Maybe something like:
>
> preset_obj = PyMapping_GetItemString(spec, "preset");
> ... parse preset ...
> ... allocate structure ...
> ... lzma_lzma_preset() ...
> static char *optnames[] = {"preset", "dict_size", "lc", "lp", NULL};
> PyArg_ParseTupleAndKeywords(
> empty_tuple, spec,
> "|OO&O&O&;invalid option for LZMA filter",
> optnames,
> &preset_obj,
> uint32_converter, &options->dict_size,
> uint32_converter, &options->lc,
> uint32_converter, &options->lp);
It turns out the issue I had in mind was that some of the fields in
the options struct are enums, whose sizes are implementation-defined.
Still, I managed to put together something not-too-ugly with some
macro hackery. The code isn't quite as neat as I had hoped, but it's
still much better than the previous revision.
Sign in to reply to this message.
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 only acceptable value) for > I think you have one extra space in your indent here. Done. Interestingly, de-indenting by one column seems to change how Sphinx formats the output. In this case I think the denser format works better, since there's not all that much text there in the first place... > http://bugs.python.org/review/6715/diff/3328/10368#newcode121 > Doc/library/lzma.rst:121: Compress *data*, returning a :class:`bytes` object > containing compressed > What type should data be? I've said that it needs to be a bytes object, but that's not strictly correct - it should accept any object that implements the buffer protocol. I'm not sure how to explain that succinctly to readers who aren't familiar with the C API, though. > http://bugs.python.org/review/6715/diff/3328/10368#newcode140 > Doc/library/lzma.rst:140: For one-shot decompression, consider using > :func:`decompress` instead. > What’s the advantage of decompress? Convenience. I've clarified the docs accordingly. > http://bugs.python.org/review/6715/diff/3328/10371#newcode3 > Lib/lzma.py:3: This module provides a file interface, classes for incremental > I don’t understand “a file interface”. I've made that bit a little more explicit, to avoid confusion. > http://bugs.python.org/review/6715/diff/3328/10371#newcode68 > Lib/lzma.py:68: When opening a file for reading, the 'preset' argument is not > To distinguish string objects from parameter names, I tend to use 'string' and > *param*. Hmm yeah, that does work better. And it also gives a nice level of consistency with the ReST markup. > http://bugs.python.org/review/6715/diff/3328/10371#newcode111 > Lib/lzma.py:111: elif mode in ("w", "a"): > You can just use one string 'wa'. No, that would also accept "" and "wa".
Sign in to reply to this message.
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"); \ a space is missing in " type"
Sign in to reply to this message.
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 large for " #TYPE "type"); \ > a space is missing in " type" Fixed. Good catch.
Sign in to reply to this message.
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. http://bugs.python.org/review/6715/diff/3335/10393#newcode364 Modules/_lzmamodule.c:364: Py_BEGIN_ALLOW_THREADS It seems that the Windows version at least is not thread-safe. If so, you would need an LZMA lock when releasing the GIL. http://bugs.python.org/review/6715/diff/3335/10393#newcode605 Modules/_lzmamodule.c:605: "have an entry for \"id\" indicating ID of the filter, plus additional\n" "indicating the ID" ?
Sign in to reply to this message.
Here are a couple of comments. I haven't take a very detailed look, but it looks generally good. Congrats! http://bugs.python.org/review/6715/diff/3465/10855 File Doc/library/lzma.rst (right): http://bugs.python.org/review/6715/diff/3465/10855#newcode117 Doc/library/lzma.rst:117: (inclusive), optionally OR-ed with the constant :const:`PRESET_EXTREME`. What is the default preset? http://bugs.python.org/review/6715/diff/3465/10858 File Lib/lzma.py (right): http://bugs.python.org/review/6715/diff/3465/10858#newcode21 Lib/lzma.py:21: "compress", "decompress", "check_is_supported", What is check_is_supported? I don't see anything in the docs. http://bugs.python.org/review/6715/diff/3465/10860 File Modules/_lzmamodule.c (right): http://bugs.python.org/review/6715/diff/3465/10860#newcode290 Modules/_lzmamodule.c:290: f->id = PyLong_AsUnsignedLongLong(id_obj); Is the cast to f->id safe? http://bugs.python.org/review/6715/diff/3465/10860#newcode695 Modules/_lzmamodule.c:695: PyObject * This function must be static. http://bugs.python.org/review/6715/diff/3465/10860#newcode831 Modules/_lzmamodule.c:831: self->lock = PyThread_allocate_lock(); If __init__ is called several times, will the old locks remain allocated? Admittedly this is a fringe case.
Sign in to reply to this message.
> Here are a couple of comments. I haven't take a very detailed look, but it looks > generally good. Congrats! Thanks! http://bugs.python.org/review/6715/diff/3465/10855 File Doc/library/lzma.rst (right): http://bugs.python.org/review/6715/diff/3465/10855#newcode117 Doc/library/lzma.rst:117: (inclusive), optionally OR-ed with the constant :const:`PRESET_EXTREME`. On 2011/10/18 18:53:14, AntoinePitrou wrote: > What is the default preset? Done. http://bugs.python.org/review/6715/diff/3465/10858 File Lib/lzma.py (right): http://bugs.python.org/review/6715/diff/3465/10858#newcode21 Lib/lzma.py:21: "compress", "decompress", "check_is_supported", On 2011/10/18 18:53:14, AntoinePitrou wrote: > What is check_is_supported? I don't see anything in the docs. Oops, missed that one. Fixed now. http://bugs.python.org/review/6715/diff/3465/10860 File Modules/_lzmamodule.c (right): http://bugs.python.org/review/6715/diff/3465/10860#newcode290 Modules/_lzmamodule.c:290: f->id = PyLong_AsUnsignedLongLong(id_obj); On 2011/10/18 18:53:14, AntoinePitrou wrote: > Is the cast to f->id safe? I think so. f->id is defined as lzma_vli, which is a typedef for uint64_t. I don't know off the top of my head whether the C standard allows for long long to be wider than 64 bits, but I would be surprised if there was a platform that actually did that. http://bugs.python.org/review/6715/diff/3465/10860#newcode695 Modules/_lzmamodule.c:695: PyObject * On 2011/10/18 18:53:14, AntoinePitrou wrote: > This function must be static. Done. http://bugs.python.org/review/6715/diff/3465/10860#newcode831 Modules/_lzmamodule.c:831: self->lock = PyThread_allocate_lock(); On 2011/10/18 18:53:14, AntoinePitrou wrote: > If __init__ is called several times, will the old locks remain allocated? > Admittedly this is a fringe case. Yes, in that case we will leak the old locks. But I don't think it's worth the effort to guard against this - AFAIK the only way it could happen would be if the user explicitly called __init__ on an already-initialized object.
Sign in to reply to this message.
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): 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 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?) 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*). You may mention that the fileobj is not closed when the LZMAFile object is destroyed or by LZMAFile.__exit__. 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`. "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). http://bugs.python.org/review/6715/diff/3506/10975#newcode59 Doc/library/lzma.rst:59: Iteration and the :keyword:`with` statement are supported. don't we have a name for the with keyword? The context manager protocol? http://bugs.python.org/review/6715/diff/3506/10975#newcode98 Doc/library/lzma.rst:98: data has not been corrupted. Possible values are: What happens if the check is not supported? An exception is raised? http://bugs.python.org/review/6715/diff/3506/10975#newcode116 Doc/library/lzma.rst:116: ``9`` (inclusive), optionally OR-ed with the constant Which value is the fastest, 0 or 9? (Which value produces the smallest output?) 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. "may"? should/must? http://bugs.python.org/review/6715/diff/3506/10975#newcode291 Doc/library/lzma.rst:291: Reading in a compressed file:: A ":" is missing in the output. You should use ": ::" or write "::" on a newline. http://bugs.python.org/review/6715/diff/3506/10975#newcode294 Doc/library/lzma.rst:294: with lzma.LZMAFile("/home/alyssa/file.xz") as f: Why do you use an absolute path? "file.xz" should be enough for an example. 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 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. http://bugs.python.org/review/6715/diff/3506/10978#newcode372 Lib/lzma.py:372: optional arguments *format*, *check*, *preset* and *filters*. reST syntax is not supported in docstring, so *bold* should be avoided. 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()) \ You can first check if val == (unsigned long)-1 http://bugs.python.org/review/6715/diff/3506/10980#newcode183 Modules/_lzmamodule.c:183: preset_obj = PyMapping_GetItemString(spec, "preset"); You may check PyMapping_Check(spec) here http://bugs.python.org/review/6715/diff/3506/10980#newcode292 Modules/_lzmamodule.c:292: if (PyErr_Occurred()) You can first check if f->id == (unsigned Py_LONG_LONG)-1 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"); may not => cannot http://bugs.python.org/review/6715/diff/3506/10980#newcode1001 Modules/_lzmamodule.c:1001: if (lzma_check_is_supported(check_id)) You can use PyBool_FromLong
Sign in to reply to this message.
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” is the official name, but a lot of docs uses “with keyword”, probably because it’s more commonly used by people and easier to look up (e.g. because “with” is a link when you use :keyword:`with`). 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. IIUC here “may not” means “cannot”. http://bugs.python.org/review/6715/diff/3506/10975#newcode291 Doc/library/lzma.rst:291: Reading in a compressed file:: Not true. Docutils replaces thing:: with thing: and creates a code block. http://bugs.python.org/review/6715/diff/3506/10975#newcode294 Doc/library/lzma.rst:294: with lzma.LZMAFile("/home/alyssa/file.xz") as f: Agreed. (Using /tmp would also be a bad idea.) http://bugs.python.org/review/6715/diff/3506/10978 File Lib/lzma.py (right): http://bugs.python.org/review/6715/diff/3506/10978#newcode372 Lib/lzma.py:372: optional arguments *format*, *check*, *preset* and *filters*. You’re confusing markup and styling :) *thing* is used to mark up arguments in Sphinx docs; I don’t think it’s a bad thing to use it in docstrings where it helps readability. (However I hate `args` that we see in shutil)
Sign in to reply to this message.
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.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
