Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add 'x' mode to lzma.open() #63400

Closed
oylenshpeegul mannequin opened this issue Oct 9, 2013 · 13 comments
Closed

Add 'x' mode to lzma.open() #63400

oylenshpeegul mannequin opened this issue Oct 9, 2013 · 13 comments
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@oylenshpeegul
Copy link
Mannequin

oylenshpeegul mannequin commented Oct 9, 2013

BPO 19201
Nosy @terryjreedy, @jcea, @vstinner, @ethanfurman, @vajrasky
Files
  • patch.lzma.py
  • add_x_mode_to_lzma.patch: Unit test for Tim Heaney's work
  • add_x_mode_to_lzma_v2.patch
  • add_x_mode_to_lzma_v3.patch
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2013-10-18.22:41:36.644>
    created_at = <Date 2013-10-09.01:31:54.742>
    labels = ['type-feature', 'library']
    title = "Add 'x' mode to lzma.open()"
    updated_at = <Date 2013-10-18.22:41:36.643>
    user = 'https://bugs.python.org/oylenshpeegul'

    bugs.python.org fields:

    activity = <Date 2013-10-18.22:41:36.643>
    actor = 'nadeem.vawda'
    assignee = 'none'
    closed = True
    closed_date = <Date 2013-10-18.22:41:36.644>
    closer = 'nadeem.vawda'
    components = ['Library (Lib)']
    creation = <Date 2013-10-09.01:31:54.742>
    creator = 'oylenshpeegul'
    dependencies = []
    files = ['32010', '32031', '32077', '32130']
    hgrepos = []
    issue_num = 19201
    keywords = ['patch']
    message_count = 13.0
    messages = ['199272', '199356', '199365', '199377', '199422', '199444', '199445', '199532', '199666', '199980', '200312', '200316', '200321']
    nosy_count = 9.0
    nosy_names = ['terry.reedy', 'jcea', 'vstinner', 'nadeem.vawda', 'Arfrever', 'ethan.furman', 'python-dev', 'vajrasky', 'oylenshpeegul']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue19201'
    versions = ['Python 3.4']

    @oylenshpeegul
    Copy link
    Mannequin Author

    oylenshpeegul mannequin commented Oct 9, 2013

    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.

    @oylenshpeegul oylenshpeegul mannequin added the type-feature A feature request or enhancement label Oct 9, 2013
    @jcea
    Copy link
    Member

    jcea commented Oct 9, 2013

    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.

    @vstinner
    Copy link
    Member

    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.

    @vajrasky
    Copy link
    Mannequin

    vajrasky mannequin commented Oct 10, 2013

    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.

    @oylenshpeegul
    Copy link
    Mannequin Author

    oylenshpeegul mannequin commented Oct 10, 2013

    Okay, I just made similar issues for gzip (bpo-19222) and bz2
    (bpo-19223). 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\>


    @Arfrever
    Copy link
    Mannequin

    Arfrever mannequin commented Oct 11, 2013

    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").

    @Arfrever
    Copy link
    Mannequin

    Arfrever mannequin commented Oct 11, 2013

    Also tarfile.open() could support "x" mode.

    @terryjreedy
    Copy link
    Member

    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').

    @terryjreedy terryjreedy changed the title lzma and 'x' mode open Add 'x' mode to lzma.open Oct 12, 2013
    @Arfrever Arfrever mannequin added the stdlib Python modules in the Lib dir label Oct 12, 2013
    @Arfrever Arfrever mannequin changed the title Add 'x' mode to lzma.open Add 'x' mode to lzma.open() Oct 12, 2013
    @vajrasky
    Copy link
    Mannequin

    vajrasky mannequin commented Oct 13, 2013

    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.

    @vajrasky
    Copy link
    Mannequin

    vajrasky mannequin commented Oct 15, 2013

    Stopped the leaking after running the test by adding self.addCleanup.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Oct 18, 2013

    New changeset b7948aaca1dd by Nadeem Vawda in branch 'default':
    Issue bpo-19201: Add support for the 'x' mode to the lzma module.
    http://hg.python.org/cpython/rev/b7948aaca1dd

    @nadeemvawda
    Copy link
    Mannequin

    nadeemvawda mannequin commented Oct 18, 2013

    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.

    @nadeemvawda
    Copy link
    Mannequin

    nadeemvawda mannequin commented Oct 18, 2013

    [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.

    @nadeemvawda nadeemvawda mannequin closed this as completed Oct 18, 2013
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants