NetHttp backend only normalizes URLs when necesary #37
Conversation
|
Awesome! I'm still really uncomfortable with the previous/current behavior, this seems to me like a step in the right direction if it still meets @janko's needs? (I also still wonder if there should be a flag to turn off this escaping altogether. The auto-escaping just seems wrong to me, in that it makes it it impossible to request certain URIs that actual HTTP servers can deliver; although I understand janko's argument that it messes people up too much when it's not there at all). |
| it "only normalizes URLs when URI says the URL is invalid" do | ||
| Addressable::URI.expects(:parse).never | ||
| Down::NetHttp.download("#{$httpbin}/etag/2ELk8hUpTC2wqJ%2BZ%25GfTFA.jpg") | ||
| end |
There was a problem hiding this comment.
I would prefer if we could test actual behaviour rather than relying on implementation. The issue was that Addressable modifies the URL into something else. So I would like the test to assert that the request made was to the original URL, instead of the different URL that Addressable's normalization would produce.
There was a problem hiding this comment.
I couldn't come up with a better way to test it, but that definitely makes sense. I updated the test so it's testing the base_uri of the tempfile.
|
@jrochkind Yes, I agree we should add the ability to disable/override URI normalization, I'll open a separate issue where we can discuss that. |
|
Looks good, thank you! I will release a new patch version with your change, then in #35 I'd like to discuss improving Addressable normalization. |
|
y'all are awesome, @janko @coding-chimp ! |
We also hit the issue mentioned in #35 today and since no-one took a crack at the proposed solution yet, I thought I'd do it. 🙂