-
-
Notifications
You must be signed in to change notification settings - Fork 30.3k
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
zlib.compress level as keyword argument #70431
Comments
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. |
Seems like a reasonable enhancement. A couple of points:
|
Thanks for the review.
|
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\>. |
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. |
The patch at bpo-16764 has a test case for the new level=... keyword. Perhaps you would like to incorporate/update it :) |
Added the "suggested" test |
New changeset 592db8704d38 by Martin Panter in branch 'default': |
New changeset dc758f51b8f5 by Martin Panter in branch 'default': |
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. |
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. |
Regenerated for review. |
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. |
New changeset 01f82a7a1250 by Serhiy Storchaka in branch 'default': |
New changeset ab2e7e51869d by Serhiy Storchaka in branch 'default': |
Thanks Berker and Martin. |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: