Allow mixing IDN and params #265
Conversation
twm
force-pushed
the
hyperlink-url-212
branch
from
11c65c4
to
5475def
Compare
| from urllib import urlencode | ||
|
|
||
| try: | ||
| # The old location was quixotically deprecated and might actually be |
There was a problem hiding this comment.
"quixotically" feels like maybe a little bit of unnecessary editorializing ;-). Do you have a link you want to drop in here?
There was a problem hiding this comment.
I have... strong feelings here.
I do not think that deprecating this import alias is a meaningful improvement to the Python standard library. This alias could be left in place, undocumented, forever, at little cost.
I have fixed this deprecation warning in countless places since it was introduced, both publicly and privately. I'm really quite tired of it. It's makework that doesn't deliver value to anyone.
I feel that this change is frankly disrespectful to library maintainers. I mean, here are some CPython devs trying to get the html5lib maintainer to do a release so that they don't break his code: html5lib/html5lib-python#419 (comment) This is not a good look.
And then, well, they eventually decide not to do it yet. Note Victor's response:
FYI [removal of] aliases to ABC in collections have been reverted in Python 3.9, but they will be removed again in Python 3.10: https://bugs.python.org/issue39674
| if isinstance(url, unicode): | ||
| parsed_url = URL.from_text(url) | ||
| else: | ||
| parsed_url = URL.from_text(url.decode('ascii')) |
There was a problem hiding this comment.
Is ascii really the right choice here? This method's supposed to return a Deferred, so this creates an awkward error-handling situation at the very least. I'd probably just go with charmap personally, but if you want a UnicodeDecodeError maybe make it asynchronous?
There was a problem hiding this comment.
Apart from the Deferred bit (which seems a defect orthogonal to the one I'm addressing here), I don't really understand the objection here. Could you file a fresh issue with a use case?
src/treq/client.py
Outdated
|
|
||
| if isinstance(url, unicode): | ||
| url = URL.fromText(url).asURI().asText().encode('ascii') | ||
| url = bytes(parsed_url) |
There was a problem hiding this comment.
Would you mind leaving the explicit .as_uri().to_text().encode() idiom here, rather than invoking it indirectly via __bytes__ / __str__? This works, but it's not documented super strictly in hyperlink and it's more or less a concession to requests' API design, not something I want to rely on really heavily.
There was a problem hiding this comment.
Sure, it looks like they're equivalent:
I'd hoped that URL might avoid some of those copies, but 🤷♂️.
| if parsed_url.query: | ||
| qs.extend([parsed_url.query, b'&']) | ||
| Parameter names and values are coerced to unicode. As a special case, | ||
| `bytes` are decoded as ASCII. |
There was a problem hiding this comment.
(Again, why ASCII rather than latin-1?)
There was a problem hiding this comment.
This matches what the urllib.parse functions did. TBH I'd rather be stricter about types than this, it's just here for compatibility with the previous implementation. Switching to charmap may make sense, but I'd want to add additional test cases to formalize the expected behavior.
|
Thank you for the review, @glyph! I will try to address the rest of your feedback soon. |
Fixes #264.