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 ruby http gem v1.0.0+ #554

Closed
wants to merge 3 commits into from
Closed

Conversation

ixti
Copy link
Contributor

@ixti ixti commented Dec 21, 2015

No description provided.

@@ -14,9 +14,17 @@ def self.from_webmock(webmock_response, request_signature = nil)
status = Status.new(webmock_response.status.first)
headers = webmock_response.headers || {}
body = Body.new Streamer.new webmock_response.body
uri = URI request_signature.uri.to_s if request_signature
uri = URI.coerce request_signature.uri.to_s if request_signature
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is failing for me.

     NoMethodError:
       undefined method `coerce' for HTTP::URI:Class

Also fails if I use ::URI.coerce

     NoMethodError:
       undefined method `coerce' for URI:Module

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh. I don't know what I was thinking of when pushed that. Thanks for catching this up.

@ixti
Copy link
Contributor Author

ixti commented Dec 21, 2015

Do not merge yet - seems like Addressable::URI changes some of the internal stuff - gonna check and will get back to this ASAP

@ixti
Copy link
Contributor Author

ixti commented Dec 22, 2015

Issue was not caused by Addressable::URI, but by WebMock appending default port to URI within request signature.

@ajb
Copy link

ajb commented Dec 28, 2015

👍 on this change -- running into this issue myself and took me a second to debug 😄

@ixti
Copy link
Contributor Author

ixti commented Dec 29, 2015

@davidbegin should be fine now :D

@davidbegin
Copy link
Collaborator

Thanks! I'll take a look today.

@davidbegin
Copy link
Collaborator

🍰 Thanks so much for fixing this! 🍰

I rebased it on master, relaxed the http rb version, and merged it in over at #559.
So I am going to close this.

@davidbegin davidbegin closed this Dec 30, 2015
@ixti ixti deleted the update-http-adapter branch December 30, 2015 12:36
@ixti
Copy link
Contributor Author

ixti commented Dec 30, 2015

Awesome! Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants