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

Jump to bottom

zlib.compress level as keyword argument #70431

Closed
palaviv mannequin opened this issue Jan 30, 2016 · 16 comments
Closed

zlib.compress level as keyword argument #70431

palaviv mannequin opened this issue Jan 30, 2016 · 16 comments
Labels
extension-modules C modules in the Modules dir type-feature A feature request or enhancement

Comments

@palaviv
Copy link
Mannequin

palaviv mannequin commented Jan 30, 2016

BPO 26243
Nosy @Yhg1s, @berkerpeksag, @vadmium, @serhiy-storchaka, @palaviv
PRs
  • bpo-26243: data= is positional-only just on CPython #11754
  • bpo-26243: data= is positional-only just on CPython #11754
  • bpo-26243: data= is positional-only just on CPython #11754
    		•
    Files
  • zlib-compress-level-keyword.patch
  • zlib-compress-level-keyword2.patch
  • zlib-compress-level-keyword3.patch: patch including test
  • zlib_compress_positional_only_data.patch
  • zlib_compress_positional_only_data.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 2016-06-25.19:48:19.446>
created_at = <Date 2016-01-30.18:41:07.895>
labels = ['extension-modules', 'type-feature']
title = 'zlib.compress level as keyword argument'
updated_at = <Date 2019-02-04.13:45:18.673>
user = 'https://github.com/palaviv'

    bugs.python.org fields:

    activity = <Date 2019-02-04.13:45:18.673>
actor = 'lukasz.langa'
assignee = 'none'
closed = True
closed_date = <Date 2016-06-25.19:48:19.446>
closer = 'serhiy.storchaka'
components = ['Extension Modules']
creation = <Date 2016-01-30.18:41:07.895>
creator = 'palaviv'
dependencies = []
files = ['41760', '41773', '41797', '43320', '43334']
hgrepos = []
issue_num = 26243
keywords = ['patch']
message_count = 16.0
messages = ['259266', '259270', '259332', '259358', '259388', '259452', '259498', '259921', '259925', '259926', '268025', '268144', '269098', '269249', '269250', '269251']
nosy_count = 7.0
nosy_names = ['twouters', 'nadeem.vawda', 'python-dev', 'berker.peksag', 'martin.panter', 'serhiy.storchaka', 'palaviv']
pr_nums = ['11754', '11754', '11754']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue26243'
versions = ['Python 3.6']
    @palaviv
    Copy link
    Mannequin Author

    palaviv mannequin commented Jan 30, 2016

    Currently zlib.compress allow only positional arguments. For code readability reasons i think that we should allow the level argument to be keyword argument. Now when someone uses zlib.compress he will be able to do zlib.compess(some_data, level=7) instead of zlib.compress(some_data, 7).

    There is a patch included with the change.

    @palaviv palaviv mannequin added type-bug An unexpected behavior, bug, or error extension-modules C modules in the Modules dir labels Jan 30, 2016
    @vadmium
    Copy link
    Member

    vadmium commented Jan 30, 2016

    Seems like a reasonable enhancement. A couple of points:

    • Watch out for the parameter names. Should it be “bytes” or “data”?
    • Z_DEFAULT_COMPRESSION is -1 not 6 (see bpo-26244)
    • Needs a version changed notice, and probably a What’s New update

    @vadmium vadmium added type-feature A feature request or enhancement and removed type-bug An unexpected behavior, bug, or error labels Jan 30, 2016
    @palaviv
    Copy link
    Mannequin Author

    palaviv mannequin commented Feb 1, 2016

    Thanks for the review.

    1. Changed "bytes" to "data".
    2. Updated Doc as in bpo-26244 (will update the patch if there will be changes there).
    3. Added version changed notice but i don't think there is a need for What's new update.

    @vadmium
    Copy link
    Member

    vadmium commented Feb 2, 2016

    Thanks, it looks good to me. I think “data” is a better name than “bytes”.

    About What’s New, I thought the general idea was to document all enhancements, though I admit this is about as small as they come. There is precedent with the compile() builtin (bpo-1444529, Python 2.6), although there is no indication of kwargs in the main documentation for compile(). But on the other hand, str.split() didn’t get any What’s New entry (bpo-14081, Python 3.3). See also <https://docs.python.org/devguide/committing.html#what-s-new-and-news-entries\>.

    @berkerpeksag
    Copy link
    Member

    There is an outdated patch in bpo-16764 for zlib.compress(), zlib.decompress(), zlib.flush() and other functions. Perhaps we can close it as a duplicate of this one.

    @vadmium
    Copy link
    Member

    vadmium commented Feb 3, 2016

    The patch at bpo-16764 has a test case for the new level=... keyword. Perhaps you would like to incorporate/update it :)

    @palaviv
    Copy link
    Mannequin Author

    palaviv mannequin commented Feb 3, 2016

    Added the "suggested" test

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Feb 9, 2016

    New changeset 592db8704d38 by Martin Panter in branch 'default':
    Issue bpo-26243: zlib.compress() keyword argument support by Aviv Palivoda
    https://hg.python.org/cpython/rev/592db8704d38

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Feb 9, 2016

    New changeset dc758f51b8f5 by Martin Panter in branch 'default':
    bpo-26243: Forgot to update zlib doc strings in Argument Clinic
    https://hg.python.org/cpython/rev/dc758f51b8f5

    @vadmium
    Copy link
    Member

    vadmium commented Feb 9, 2016

    I did add a What’s New entry (I hope nobody minds), and updated the doc strings in the zlib module. Thanks for the work on this Aviv.

    @vadmium vadmium closed this as completed Feb 9, 2016
    @serhiy-storchaka
    Copy link
    Member

    The original request was for supporting level as keyword argument. Making the first argument a keyword argument was unintentional side effect (due a to the limitation of argument parsing functions). Now it is possible to support positional-only and keyword arguments in one function (see bpo-26282). Proposed patch makes the first argument positional-only again.

    @serhiy-storchaka
    Copy link
    Member

    Regenerated for review.

    @vadmium
    Copy link
    Member

    vadmium commented Jun 23, 2016

    The patch (with Berker’s fix) looks okay. Personally, I don’t see a big problem with the first argument also having a keyword name, but I don’t mind if it doesn’t either.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jun 25, 2016

    New changeset 01f82a7a1250 by Serhiy Storchaka in branch 'default':
    Issue bpo-26243: Only the level argument to zlib.compress() is keyword argument
    https://hg.python.org/cpython/rev/01f82a7a1250

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jun 25, 2016

    New changeset ab2e7e51869d by Serhiy Storchaka in branch 'default':
    Issue bpo-26243: Correct a wording in docs.
    https://hg.python.org/cpython/rev/ab2e7e51869d

    @serhiy-storchaka
    Copy link
    Member

    Thanks Berker and Martin.

    @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
    Assignees
    No one assigned
    Labels
    extension-modules C modules in the Modules dir type-feature A feature request or enhancement
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    3 participants
    @berkerpeksag @vadmium @serhiy-storchaka