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

urllib data URL #60627

Closed
panzi mannequin opened this issue Nov 7, 2012 · 15 comments
Closed

urllib data URL #60627

panzi mannequin opened this issue Nov 7, 2012 · 15 comments
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@panzi
Copy link
Mannequin

panzi mannequin commented Nov 7, 2012

BPO 16423
Nosy @orsenthil, @pitrou
Files
  • doc-urllib.request-data-url-recipe.patch: Patch that adds a data URL handler recipe to the urllib documentation.
  • urllib.request-data-url.patch: urllib.request patch for data url support (w/ doc and tests)
    		•

    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 2012-11-24.17:03:24.371>
created_at = <Date 2012-11-07.03:28:01.908>
labels = ['invalid', 'type-feature', 'library']
title = 'urllib data URL'
updated_at = <Date 2012-11-24.18:39:10.366>
user = 'https://bugs.python.org/panzi'

    bugs.python.org fields:

    activity = <Date 2012-11-24.18:39:10.366>
actor = 'pitrou'
assignee = 'none'
closed = True
closed_date = <Date 2012-11-24.17:03:24.371>
closer = 'pitrou'
components = ['Library (Lib)']
creation = <Date 2012-11-07.03:28:01.908>
creator = 'panzi'
dependencies = []
files = ['27914', '28018']
hgrepos = ['160']
issue_num = 16423
keywords = ['patch']
message_count = 15.0
messages = ['175041', '175667', '175828', '175831', '175833', '175840', '175842', '175867', '176294', '176295', '176298', '176299', '176300', '176304', '176305']
nosy_count = 5.0
nosy_names = ['orsenthil', 'pitrou', 'panzi', 'docs@python', 'python-dev']
pr_nums = []
priority = 'normal'
resolution = 'not a bug'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue16423'
versions = ['Python 3.4']
    @panzi
    Copy link
    Mannequin Author

    panzi mannequin commented Nov 7, 2012

    I think it would be really helpful if urllib would support data URLs. However, I was told on the python-ideas mailing list that it would probably only added as recipe in the documentation. The attached patch adds such an recipe to the urllib.request documentation. I've never written a doc patch (or any patch) for python before, so I don't know if I added it at the right place in the right format.

    @panzi panzi mannequin assigned docspython Nov 7, 2012
    @panzi panzi mannequin added the docs Documentation in the Doc dir label Nov 7, 2012
    @orsenthil orsenthil assigned orsenthil and unassigned docspython Nov 7, 2012
    @panzi
    Copy link
    Mannequin Author

    panzi mannequin commented Nov 16, 2012

    New patch. Instead of adding the data URL support to the doc as a recipe I added it to urllib.request directly. I think this is better and justified, because the old legacy URLopener had (some kind) of support for data URLs.

    OT: I think that the legacy URLopener has bugs. It only unquotes the data if it's base64 encoded, but it should be unquoted in any case. Also it returns a StringIO and decodes the content as latin-1. This is wrong, the content is a binary string. It could be text encoded in any kind of charset or not text at all (e.g. an image).

    @panzi panzi mannequin added stdlib Python modules in the Lib dir and removed docs Documentation in the Doc dir labels Nov 16, 2012
    @panzi panzi mannequin changed the title urllib data URL recipe urllib data URL Nov 16, 2012
    @panzi panzi mannequin added the type-feature A feature request or enhancement label Nov 16, 2012
    @pitrou
    Copy link
    Member

    pitrou commented Nov 17, 2012

    A couple of comments:

    • the patch needs a test (and docs too)
    • are you sure ignoring POSTed data is the right thing to do? Shouldn't we forbid it instead?
    • I think it would be nice to reference the RFC number somewhere
    • not sure why you raise IOError on a bad URL; I would say ValueError is the right exception here

    +1 on the general idea, by the way.

    @panzi
    Copy link
    Mannequin Author

    panzi mannequin commented Nov 17, 2012

    On 11/18/2012 12:36 AM, Antoine Pitrou wrote:

    Antoine Pitrou added the comment:

    A couple of comments:

    • the patch needs a test (and docs too)

    Will do (when I have time).

    • are you sure ignoring POSTed data is the right thing to do? Shouldn't we forbid it instead?
    • I think it would be nice to reference the RFC number somewhere
    • not sure why you raise IOError on a bad URL; I would say ValueError is the right exception here

    I did that because that's what the old URLopener code does (ignoring POSTed data and raising an
    IOError). The comment is actually a 1:1 copy of the old code.

    +1 on the general idea, by the way.

    ----------
    nosy: +pitrou

    Python tracker <report@bugs.python.org>
    <http://bugs.python.org/issue16423\>

    @pitrou
    Copy link
    Member

    pitrou commented Nov 17, 2012

    > - are you sure ignoring POSTed data is the right thing to do? Shouldn't we forbid it instead?
    > - I think it would be nice to reference the RFC number somewhere
    > - not sure why you raise IOError on a bad URL; I would say ValueError is the right exception here
    >

    I did that because that's what the old URLopener code does (ignoring POSTed data and raising an
    IOError). The comment is actually a 1:1 copy of the old code.

    Ok. I think it's better to take a fresh start here :-)

    @panzi
    Copy link
    Mannequin Author

    panzi mannequin commented Nov 18, 2012

    On 11/18/2012 12:36 AM, Antoine Pitrou wrote:

    • the patch needs a test (and docs too)
    • are you sure ignoring POSTed data is the right thing to do? Shouldn't we forbid it instead?

    Btw.: The file:// protocol handler also just ignores posted data, so I think the data url handler
    shouldn't deviate from that. Either both raise an error when data is posted or none.

    @panzi
    Copy link
    Mannequin Author

    panzi mannequin commented Nov 18, 2012

    Ok, I've added a documentation and some tests. Is it ok this way? More tests? More/other documentation?

    @pitrou
    Copy link
    Member

    pitrou commented Nov 18, 2012

    On 11/18/2012 12:36 AM, Antoine Pitrou wrote:
    >
    > - the patch needs a test (and docs too)
    > - are you sure ignoring POSTed data is the right thing to do? Shouldn't we forbid it instead?

    Btw.: The file:// protocol handler also just ignores posted data, so I think the data url handler
    shouldn't deviate from that. Either both raise an error when data is posted or none.

    Ah, fair enough, then. Thanks for the updated patch, I'll take a look
    (unless someone beats me to it).

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Nov 24, 2012

    New changeset a182367eac5a by Antoine Pitrou in branch 'default':
    Issue bpo-16423: urllib.request now has support for ``data:`` URLs.
    http://hg.python.org/cpython/rev/a182367eac5a

    @pitrou
    Copy link
    Member

    pitrou commented Nov 24, 2012

    I've committed your patch after having made the few very minor changes mentioned in the review. Thank you very much!

    @pitrou pitrou closed this as completed Nov 24, 2012
    @pitrou pitrou added the invalid label Nov 24, 2012
    @panzi
    Copy link
    Mannequin Author

    panzi mannequin commented Nov 24, 2012

    Great!

    Feels awesome to have my first bit of code contributed to the Python project. :)

    @pitrou
    Copy link
    Member

    pitrou commented Nov 24, 2012

    You're welcome! We're always happy to have new contributors.

    I've forgotten something: could you sign a contributor agreement?
    You'll find instructions at http://www.python.org/psf/contrib/

    @panzi
    Copy link
    Mannequin Author

    panzi mannequin commented Nov 24, 2012

    Will do (later today).

    @panzi
    Copy link
    Mannequin Author

    panzi mannequin commented Nov 24, 2012

    Hmm, which of the two initial licenses should I choose? Which one do you rather want me to choose?

    @pitrou
    Copy link
    Member

    pitrou commented Nov 24, 2012

    Hmm, which of the two initial licenses should I choose? Which one do
    you rather want me to choose?

    Whichever you prefer. They should be equivalent in their terms
    (non-copyleft free licenses).

    @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
    stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    2 participants
    @orsenthil @pitrou