Use can_ada for supported URLs - #261
AdrianAtZyte wants to merge 11 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #261 +/- ##
===========================================
- Coverage 100.00% 98.87% -1.13%
===========================================
Files 10 10
Lines 1042 1067 +25
Branches 231 235 +4
===========================================
+ Hits 1042 1055 +13
- Misses 0 6 +6
- Partials 0 6 +6
🚀 New features to boost your workflow:
|
| ) | ||
| assert isinstance(safeurl, str) | ||
| assert safeurl == "http://www.example.com/%C2%A3?unit=%B5" | ||
| assert safeurl == "http://www.example.com/%C2%A3?unit=%C2%B5" |
There was a problem hiding this comment.
There are several changes that look like it was latin1 and now is utf-8, and in at least some of them an arg with "latin1" is passed to the function, I wonder if all of these are still valid behavior.
There was a problem hiding this comment.
My understanding is that latin1 is used for decoding bytes to str, then utf-8 for percent-encoding, which I think aligns with WHATWG.
Yes, it says that explicitly so maybe we don't want to have those tests... |
|
I'd keep can_ada as an optional dependency and also used it's |
|
Any specific reason in mind for making it optional? I’m not against it, but I would usually do that only if I can think of a good reason why someone would not want it installed (e.g. not supporting or being hard to install on some platform). I did not check if can_ada can be easily installed on Windows, and I did notice that the last commit was from 6 months ago which may be worrisome (although maybe we could assume maintenance in a worst-case scenario if it seems worth it, which it does to me). I wonder if you have other reasons in mind. |
|
Before this PR, w3lib had no runtime dependencies at all. This would introduce the first one, and it’s also a compiled extension. Some users may be relying on w3lib specifically because it’s dependency-free and works everywhere without requiring a compiler or prebuilt wheels. I’m not saying we shouldn’t use can_ada, just that changing those assumptions can have downstream effects on existing environments and build pipelines. To me, that’s either something for a major version bump, or a good case for making it an optional dependency so existing users aren’t affected unless they opt in. |
# Conflicts: # tests/test_url.py
#221 (comment)
Fixes scrapy/scrapy#1306, fixes #98, fixes #204, closes #221, fixes #222, closes #270.
Some of the tests in #259 would still not pass with these changes, but I suspect that may be OK (can_ada / WHATWG diverge from stdlib).