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

Conversation

@joealcorn
Copy link
Contributor

commented Sep 20, 2014

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor

commented Sep 20, 2014

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 joealcorn force-pushed the joealcorn:byte-urls branch from c4b735b to 9b3f3e4 Sep 29, 2014

@joealcorn

This comment has been minimized.

Copy link
Contributor Author

commented Sep 29, 2014

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?

@joealcorn joealcorn force-pushed the joealcorn:byte-urls branch from 9b3f3e4 to aaa458d Sep 29, 2014

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

This comment has been minimized.

Copy link
@sigmavirus24

sigmavirus24 Sep 30, 2014

Contributor

Would this be acceptable:

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

This comment has been minimized.

Copy link
@joealcorn

joealcorn Sep 30, 2014

Author Contributor

That's nice and succinct, have ammended and pushed

@joealcorn joealcorn force-pushed the joealcorn:byte-urls branch from aaa458d to a68d1b4 Sep 30, 2014

@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

This comment has been minimized.

Copy link
Contributor

commented Sep 30, 2014

❤️ @buttscicles

@Lukasa this looks okay to me. Thoughts?

@Lukasa

This comment has been minimized.

Copy link
Member

commented Sep 30, 2014

🍰 Make it so.

@kennethreitz

This comment has been minimized.

Copy link
Contributor

commented Oct 2, 2014

Let's not document this :)

kennethreitz added a commit that referenced this pull request Oct 2, 2014
Merge pull request #2238 from buttscicles/byte-urls
Support bytestring URLs on Python 3.x

@kennethreitz kennethreitz merged commit e74791d into psf:master Oct 2, 2014

@joealcorn joealcorn deleted the joealcorn:byte-urls branch Oct 3, 2014

benjaminp added a commit to benjaminp/typeshed that referenced this pull request Aug 28, 2019
requests: Allow bytes for url parameters.
typeshed already partially reflected psf/requests#2238 but not completely.
msullivan added a commit to python/typeshed that referenced this pull request Aug 29, 2019
requests: Allow bytes for url parameters. (#3209)
typeshed already partially reflected psf/requests#2238 but not completely.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.