Unicode support for urlparse 4 #3
Conversation
|
@lopuhin, this PR should be clearer than the one in the original repository 😄 |
There was a problem hiding this comment.
@nctl144 I think this is a good start, but would be also great to add tests which check unicode support, and also it would be great to first add travis.ci and tox support to the repo before proceeding with more changes (but that's a topic for another PR) - it greatly simplifies testing and reviewing.
urlparse4/cgurl.pyx
Outdated
| slice_component(url, parsed.query), | ||
| slice_component(url, parsed.ref) | ||
| )) | ||
| if six.PY2: |
There was a problem hiding this comment.
Two points about this bit of code:
- it would be nice to avoid duplication of code between python 2 and 3
- I'm not sure
unicodefunction is right here, from the docs: "Create a new Unicode object from the given encoded string. Encoding defaults to the current default string encoding." - I don't think we should rely on default encoding. Also, if we need to do encoding/decoding, it's more efficient to do this in Cython
There was a problem hiding this comment.
Regarding returning unicode or not, stdlib urlparse functions can return unicode, so to me it looks more sane to always return unicode even on python 2, maybe it will also be easier to support. Although this might require some extra effort when doing scrapy integration, see e.g. scrapy/scrapy#1949 (comment)
|
You are right! I will set up tox and travis in the next PRs :D |
No description provided.