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

Support bytestring URLs on Python 3.x #2238

Merged
merged 1 commit into from Oct 2, 2014
Merged

Conversation

joealcorn
Copy link
Contributor

Hi there folks.

Currently prepare_url will call unicode or str on the url arg depending on the python version. This works fine for most cases, but the one case it trips up on is bytestrings on python 3.x as the string representation of these is "b'http://httpbin.org'". Eventually this will surface as an InvalidSchema exception.

I find this to be completely unexpected, and I'd imagine it's not something that's been done intentionally.

Technically this a breaking change. The possibility of passing non-strings to prepare_url is undocumented and untested, but regardless it may be better to go about fixing this in a different way, that's your call.

@Lukasa
Copy link
Member

Lukasa commented Sep 20, 2014

Thanks for this!

I think we like the ability to pass non-strings to prepare_url. It allows people to use custom url-building classes without running into trouble. It's worth noting that you change also breaks Python 2 behaviour: previously a Python 2 str would get lifted to a Python 2 unicode, which it now doesn't do.

I think we can get around this by simply special-casing string types. The logic is really:

if type is unicode, leave unchanged
else, if type is bytes, decode bytestring to unicode
else, if type is anything else, call the 'to unicode' method

I think that's really the logic we want here. @sigmavirus24, thoughts?

@sigmavirus24
Copy link
Contributor

I agree with @Lukasa that this is the behaviour we want. We absolutely want unicode urls because on Python 3, we can handle IRIs. We would need to adopt something that implements RFC 3987 to support it on Python 2, but once we did, we would be able to support them there too. For example, http://☃.net should be supported. (Your browser, and Python 3 should properly "encode" that to http://xn--n3h.net/.)

@joealcorn
Copy link
Contributor Author

Ah good point, forgot about that use case.
Have updated the branch, I believe it's doing what it should be now, how's that?

#: as this will include the bytestring indicator (b'')
#: on python 3.x.
#: https://github.com/kennethreitz/requests/pull/2238
if not is_py2:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this be acceptable:

try:
    url = url.decode('utf8')
except AttributeError:
    url = unicode(url) if not is_py2 else str(url)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's nice and succinct, have ammended and pushed

@joealcorn joealcorn changed the title Call to_native_string on urls passed to prepare_url Support bytestring URLs on Python 3.x Sep 30, 2014
@sigmavirus24
Copy link
Contributor

❤️ @buttscicles

@Lukasa this looks okay to me. Thoughts?

@Lukasa
Copy link
Member

Lukasa commented Sep 30, 2014

🍰 Make it so.

@kennethreitz
Copy link
Contributor

Let's not document this :)

kennethreitz added a commit that referenced this pull request Oct 2, 2014
Support bytestring URLs on Python 3.x
@kennethreitz kennethreitz merged commit e74791d into psf:master Oct 2, 2014
@joealcorn joealcorn deleted the byte-urls branch October 3, 2014 02:06
benjaminp added a commit to benjaminp/typeshed that referenced this pull request Aug 28, 2019
typeshed already partially reflected psf/requests#2238 but not completely.
msullivan pushed a commit to python/typeshed that referenced this pull request Aug 29, 2019
typeshed already partially reflected psf/requests#2238 but not completely.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants