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

Add an optional newline parameter to binascii.b2a_base64() to not add a newline #69544

Closed
vstinner opened this issue Oct 9, 2015 · 9 comments
Closed

Add an optional newline parameter to binascii.b2a_base64() to not add a newline #69544

vstinner opened this issue Oct 9, 2015 · 9 comments
Labels
performance Performance or resource usage

Comments

@vstinner
Copy link
Member

vstinner commented Oct 9, 2015

BPO 25357
Nosy @ncoghlan, @pitrou, @vstinner, @vadmium, @serhiy-storchaka
Files
  • binascii_b2a_base64_newline.patch
  • binascii_b2a_base64_newline-2.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 2015-10-11.09:02:38.115>
created_at = <Date 2015-10-09.16:21:56.846>
labels = ['performance']
title = 'Add an optional newline parameter to binascii.b2a_base64() to not add a newline'
updated_at = <Date 2015-10-11.09:02:38.114>
user = 'https://github.com/vstinner'

    bugs.python.org fields:

    activity = <Date 2015-10-11.09:02:38.114>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2015-10-11.09:02:38.115>
closer = 'vstinner'
components = []
creation = <Date 2015-10-09.16:21:56.846>
creator = 'vstinner'
dependencies = []
files = ['40731', '40733']
hgrepos = []
issue_num = 25357
keywords = ['patch']
message_count = 9.0
messages = ['252625', '252627', '252630', '252631', '252632', '252684', '252692', '252791', '252792']
nosy_count = 6.0
nosy_names = ['ncoghlan', 'pitrou', 'vstinner', 'python-dev', 'martin.panter', 'serhiy.storchaka']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = None
status = 'closed'
superseder = None
type = 'performance'
url = 'https://bugs.python.org/issue25357'
versions = ['Python 3.6']
    @vstinner
    Copy link
    Member Author

    vstinner commented Oct 9, 2015

    The base64.b64encode() function calls binascii.b2a_base64() and then strips the newline. It would be more efficient to directly not add a newline.

    Attached patch adds an optional newline parameter to binascii.b2a_base64(). It also modifies base64.b64encode() to call binascii.b2a_base64() with newline=False.

    @vstinner vstinner added the performance Performance or resource usage label Oct 9, 2015
    @serhiy-storchaka
    Copy link
    Member

    +1. Not sure about the parameter name, but if no one suggests better, it LGTM. Added comments on Rietveld.

    What about adding the same parameter to binascii.b2a_uu()?

    @vstinner
    Copy link
    Member Author

    vstinner commented Oct 9, 2015

    Oh, I forgot to add an unit test.

    What about adding the same parameter to binascii.b2a_uu()?

    This function is used in Lib/encodings/uu_codec.py and Lib/uu.py, but the newline is not stripped. So I don't think that it's worth to add an optional parameter.

    @vstinner
    Copy link
    Member Author

    vstinner commented Oct 9, 2015

    Updated patch to take Serhiy's comments in account and now with an unit test!

    @vstinner
    Copy link
    Member Author

    vstinner commented Oct 9, 2015

    Oh by the way, the new parameter is a keyword only parameter. It's not possible to write b2a_base64(data, False). Maybe I should also add an unit test for this.

    @vadmium
    Copy link
    Member

    vadmium commented Oct 10, 2015

    The “newline” name seems as good as any. The patch looks good in general. I left a few comments.

    I agree with not bothering for the UU encoder. That encoding has an embedded line length byte at the start of each line, so it makes less sense to omit newlines. Base-64 is used in places such as URLs where multiline output is not relevant, but I don’t know of a similar case for the UU encoding.

    If I was doing it, I wouldn’t bother with a test case for keyword-only-ness, but suit yourself.

    @serhiy-storchaka
    Copy link
    Member

    With Martin's comments the patch LGTM.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Oct 11, 2015

    New changeset 463a09a3bfff by Victor Stinner in branch 'default':
    Issue bpo-25357: Add an optional newline paramer to binascii.b2a_base64().
    https://hg.python.org/cpython/rev/463a09a3bfff

    @vstinner
    Copy link
    Member Author

    Thanks for reviews Serhiy & Martin.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @FoamyGuy FoamyGuy mentioned this issue Mar 19, 2024
    Merged
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees
    No one assigned
    Labels
    performance Performance or resource usage
    Projects
    None yet
    Milestone
    No milestone
    Development

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