classification
Title: Add 'x' mode to lzma.open()
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Arfrever, ethan.furman, haypo, jcea, nadeem.vawda, oylenshpeegul, python-dev, terry.reedy, vajrasky
Priority: normal Keywords: patch

Created on 2013-10-09 01:31 by oylenshpeegul, last changed 2013-10-18 22:41 by nadeem.vawda. This issue is now closed.

Files
File name Uploaded Description Edit
patch.lzma.py oylenshpeegul, 2013-10-09 01:31
add_x_mode_to_lzma.patch vajrasky, 2013-10-10 10:33 Unit test for Tim Heaney's work review
add_x_mode_to_lzma_v2.patch vajrasky, 2013-10-13 07:50 review
add_x_mode_to_lzma_v3.patch vajrasky, 2013-10-15 08:03 review
Messages (13)
msg199272 - (view) Author: Tim Heaney (oylenshpeegul) Date: 2013-10-09 01:31
I love the 'x' mode open in recent versions of Python. I just discovered that lzma.open doesn't support it. It seems there's an elif that explicitly checks the modes allowed. I added "x" and "xb" to the choices and now it works as I would like.
msg199356 - (view) Author: Jesús Cea Avión (jcea) * (Python committer) Date: 2013-10-09 23:20
Looks good. Being strict this would be 3.4 material, but patch is trivial and looks like a oversight. We should check other compression modules like gzip, bzip2, etc.
msg199365 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2013-10-10 07:43
> Being strict this would be 3.4 material,

Why? The patch is trivial, I don't how it could cause a regression.

If you don't want regression, add a unit test to test_lzma.py.
msg199377 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2013-10-10 10:31
Here is the unit test for Tim Heaney's work. There is a test that explicitly tests that we get error when opening in 'x' mode. Also, this test is only for lzma. I think we should create separate tickets for other compression methods.
msg199422 - (view) Author: Tim Heaney (oylenshpeegul) Date: 2013-10-10 21:45
Okay, I just made similar issues for gzip (issue19222) and bz2
(issue19223). It's weird how different these three patches are! We're
essentially doing the same thing: "please allow the x option to pass
through to builtins.open." Why don't these three modules look more alike?

On Thu, Oct 10, 2013 at 6:31 AM, Vajrasky Kok <report@bugs.python.org>wrote:

>
> Vajrasky Kok added the comment:
>
> Here is the unit test for Tim Heaney's work. There is a test that
> explicitly tests that we get error when opening in 'x' mode. Also, this
> test is only for lzma. I think we should create separate tickets for other
> compression methods.
>
> ----------
> keywords: +patch
> nosy: +vajrasky
> Added file: http://bugs.python.org/file32030/add_x_mode_to_lzma.patch
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue19201>
> _______________________________________
>
msg199444 - (view) Author: Arfrever Frehtes Taifersar Arahesis (Arfrever) * Date: 2013-10-11 07:39
Is there any reason why the order of characters matters here?
builtins.open() supports them in any order ("br"=="rb", "bw"=="wb", "ba"=="ab", "bx"=="xb").
msg199445 - (view) Author: Arfrever Frehtes Taifersar Arahesis (Arfrever) * Date: 2013-10-11 07:50
Also tarfile.open() could support "x" mode.
msg199532 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2013-10-12 01:30
The bug versus feature issue does not depend on whether the patch could cause a regression. (I think feature patches might actually be less likely than bug fixes to cause regressions, but it does not matter.) Nor is oversight, not adding a feature when it could have been added, a bug. The point is that adding a new feature in a bug-fix release makes it a feature release, and this is a new feature. Code that uses the new feature will not run on previous releases of the same version.

The doc for lzma.open says "The mode argument can be any of "r", "rb", "w", "wb", "a" or "ab" for binary mode, or "rt", "wt", or "at" for text mode. The default is "rb"." (I assume that) the code does just what the doc says. (If it did not, *that* would be a bug).

Arfrever's point about the order of characters makes me wonder why mode strings (as opposed to characters in the strings) are being checked. The following tests that exactly one of w, a, x appear in mode.
  if len({'w', 'a', 'x'} & set(mode)) == 1:
If mode is eventually passed to open(), the latter would do what ever it does with junk chars in mode (such as 'q').
msg199666 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2013-10-13 07:50
Added doc. Revamped the test. The patch did not cater to the order of modes ("wb" is equal to "bw"?). I think that deserves a separate ticket.
msg199980 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2013-10-15 08:03
Stopped the leaking after running the test by adding self.addCleanup.
msg200312 - (view) Author: Roundup Robot (python-dev) Date: 2013-10-18 22:11
New changeset b7948aaca1dd by Nadeem Vawda in branch 'default':
Issue #19201: Add support for the 'x' mode to the lzma module.
http://hg.python.org/cpython/rev/b7948aaca1dd
msg200316 - (view) Author: Nadeem Vawda (nadeem.vawda) * (Python committer) Date: 2013-10-18 22:22
Fix committed. Thanks for the patches!

As Jesús and Terry have said, this won't be backported to 3.3/2.7, since
it is a new feature.


[oylenshpeegul]
| It's weird how different these three patches are! We're
| essentially doing the same thing: "please allow the x option to pass
| through to builtins.open." Why don't these three modules look more alike?

Mostly because they were written at different times, by different people,
with different things to be backward-compatible with. Ideally they would
share the bulk of their code, but it's tricky to do that without changing
behavior in some corner cases.
msg200321 - (view) Author: Nadeem Vawda (nadeem.vawda) * (Python committer) Date: 2013-10-18 22:41
[terry.reedy]
| Arfrever's point about the order of characters makes me wonder why mode
| strings (as opposed to characters in the strings) are being checked.
| The following tests that exactly one of w, a, x appear in mode.
|  if len({'w', 'a', 'x'} & set(mode)) == 1:
| If mode is eventually passed to open(), the latter would do what ever
| it does with junk chars in mode (such as 'q').

There are two separate questions here - how rigid we are about modes
containing only valid characters, and how we handle invalid characters.

I don't think there's any point in passing through unrecognized chars
to builtins.open(), since it results in a ValueError either way.

On the first point, the code only accepts modes like 'r' and 'rb' (but
not 'br') for the sake of simplicity. There doesn't seem to be much
practical value in accepting arbitrarily-ordered modes, but if someone
has a compelling use-case (or a patch that's no more complex than the
status quo), please bring it up in a separate issue.
History
Date User Action Args
2013-10-18 22:41:36nadeem.vawdasetstatus: open -> closed
resolution: fixed
messages: + msg200321

stage: patch review -> resolved
2013-10-18 22:22:49nadeem.vawdasetmessages: + msg200316
2013-10-18 22:11:37python-devsetnosy: + python-dev
messages: + msg200312
2013-10-15 08:03:45vajraskysetfiles: + add_x_mode_to_lzma_v3.patch

messages: + msg199980
2013-10-13 07:50:33vajraskysetfiles: + add_x_mode_to_lzma_v2.patch

messages: + msg199666
2013-10-12 08:32:59Arfreversetcomponents: + Library (Lib)
title: Add 'x' mode to lzma.open -> Add 'x' mode to lzma.open()
2013-10-12 01:30:37terry.reedysetnosy: + terry.reedy
title: lzma and 'x' mode open -> Add 'x' mode to lzma.open
messages: + msg199532

versions: - Python 3.3
2013-10-11 07:50:08Arfreversetmessages: + msg199445
2013-10-11 07:39:53Arfreversetnosy: + Arfrever
messages: + msg199444
2013-10-10 21:45:24oylenshpeegulsetmessages: + msg199422
2013-10-10 10:33:29vajraskysetfiles: + add_x_mode_to_lzma.patch
2013-10-10 10:33:14vajraskysetfiles: - add_x_mode_to_lzma.patch
2013-10-10 10:31:45vajraskysetfiles: + add_x_mode_to_lzma.patch

nosy: + vajrasky
messages: + msg199377

keywords: + patch
2013-10-10 07:43:57hayposetnosy: + haypo
messages: + msg199365
2013-10-09 23:20:29jceasetnosy: + jcea
messages: + msg199356
2013-10-09 16:13:07ethan.furmansetnosy: + ethan.furman

stage: patch review
2013-10-09 01:52:26benjamin.petersonsetnosy: + nadeem.vawda
2013-10-09 01:44:32oylenshpeegulsetversions: + Python 3.3
2013-10-09 01:31:54oylenshpeegulcreate