Updates hook_into to http_client, preserves backwards compatibility #228

Closed
wants to merge 2 commits into
from

Projects

None yet

2 participants

@brycemcd
brycemcd commented Dec 3, 2012

A simple PR to update the hook_into configuration to be called http_client or http_clients instead. hook_into is perfectly reasonable as a configuration method, but led to some confusion when I was skimming the docs and ultimately led to the SO post here:
http://stackoverflow.com/questions/13665667/vcr-not-recording-cassettes#comment18755899_13665667

This PR simply renames the hook_into method to http_client and aliases hook_into to preserve backwards compatibility.

brycemcd added some commits Dec 3, 2012
@brycemcd brycemcd updates hook_into config to be http_client(s)
  * hook_into aliased to preserve backward compatibility
  * updates specs to cover all three invocations
025ecc5
@brycemcd brycemcd updates readme 42f6bc9
@myronmarston
Member

@brycem -- Thanks for the pull request!

I understand the confusion, but I think the new name you've suggested (http_client) will cause additional confusion. c.http_clients :typhoeus, :excon makes sense since both Typhoeus and Excon are HTTP clients, but c.http_client :webmock doesn't make much sense (nor does http_client :fakeweb) since they aren't HTTP clients. On top of that, there's already a deprecated c.stub_with :fakeweb, :typhoeus method (from VCR 1.x); given that that still works, I'd rather not add 2 more alias (http_client and http_clients). I think users will find it confusing that there are 4 config methods that all do the same thing.

BTW, it's not clear to me why http_client :typhoeus is any less confusing than hook_into :typhoeus...can you explain your thinking here?

@brycemcd
brycemcd commented Dec 7, 2012

My thinking with hook_into :typhoeus was because I wasn't exactly clear what hook_into was really doing. Now that I've spent some time using VCR it makes more sense, but when I first began, I thought VCR was using :webmock or :fakeweb (or whatever) to rebuild the request from the cassette that was created regardless of which http client library was used originally to actually create the cassette. It was conceivable to me that VCR stubbed out something like a socket response using whatever was provided by hook_into that the actual http_client library then read and parsed just as if it had actually hit the network.

http_client, to me, is more intuitive in the way testing and VCR work (though you make a very good point that neither :fakeweb or :webmock are http clients and I completely agree that that would be confusing). The way I read c.http_client is "give me the http client you're using in your code. I'll mock a response object using your library without touching the network so you can test the code that depends on the response."

hook_into now makes more sense, but I think it's not crystal clear that hook_into is not a consideration of an internal API of VCR but rather a communication into VCR about which http client is being used to make network requests in the actual code under test.

@myronmarston
Member

I can understand confusion over how VCR works internally, and how hook_into relates to that...but as we've agreed, http_client has its own sets of confusions. I don't want to change (or multiply) an API if it's not a clear win (particularly given that I've never heard other users complain about this).

I'm going to close this (as I don't plan to merge it) but please contribute again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment