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

add support for yielding HTTP::Client method types #27

Merged
merged 1 commit into from
Oct 7, 2019

Conversation

repomaa
Copy link
Contributor

@repomaa repomaa commented Oct 1, 2018

i.e.

WebMock.stub(:get, "http://www.foo.bar").to_return(body_io: IO::Memory.new("foobar"))

HTTP::Client.get("http://www.foo.bar") do |response|
  File.open("dump", "w+") do |file|
    IO.copy(response.body_io, file)
  end
end

@wezm
Copy link

wezm commented Oct 7, 2019

This PR would be useful for a project I'm working on. Anything needed (aside from perhaps resolving conflicts) to get it over the line?

@bcardiff
Copy link
Contributor

bcardiff commented Oct 7, 2019

@repomaa there are a couple of conflicts preventing this to be merged.

I also found confusing the description of the PR w.r.t. the specs added. All the example are about been able to set the response io, and that does not require the user of webmock to use yields or blocks. Am I overlooking something?

@repomaa
Copy link
Contributor Author

repomaa commented Oct 7, 2019

@bcardiff you might be right about the confusion regarding the specs. The thing is that you can only access body_io on the yielding variant of HTTP::Client#get.

@bcardiff
Copy link
Contributor

bcardiff commented Oct 7, 2019

When possible I find it clearer to express things from the perspective for the user "Allow stubbing body IO (aka: file payloads)" rather than the implementation "Add X/Y overload".

Thanks for updating the PR @repomaa !

@bcardiff bcardiff merged commit 78bb0e3 into manastech:master Oct 7, 2019
@wezm
Copy link

wezm commented Oct 7, 2019

Great, thank you both for getting this over the line.

@msa7
Copy link
Contributor

msa7 commented Mar 26, 2020

I have an issue #41 after these changes. Any suggestions how to fix?

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