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

Webmock broken by new HTTP::Response#initialize API #277

Closed
tarcieri opened this issue Dec 19, 2015 · 11 comments
Closed

Webmock broken by new HTTP::Response#initialize API #277

tarcieri opened this issue Dec 19, 2015 · 11 comments
Assignees
Milestone

Comments

@tarcieri
Copy link
Member

See f40fbda: Use opts hash for HTTP::Response.new

This seems like a good change. I think we should release with this new API and update Webmock accordingly.

cc @Connorhd @nerdrew @zanker

@zanker
Copy link
Contributor

zanker commented Dec 19, 2015

Sounds good. I'd like to see us do this in a way that's more backwards compatible but I'm not exactly sure if that's feasible.

@ixti
Copy link
Member

ixti commented Dec 19, 2015

Oh. I merged that and forgot to provide webmock adapter update. Will do that this weekend.

@ixti ixti self-assigned this Dec 19, 2015
@ixti
Copy link
Member

ixti commented Dec 19, 2015

I don't see a way to make it backward compatible either. But I don't see worth in keeping API backward compatible - we just removed bunch of methods ;D we're braking bad already :D

@tarcieri
Copy link
Member Author

Sidebar: if we do this for Response, should we do it for Request too?

https://github.com/httprb/http/blob/master/lib/http/request.rb#L67

@ixti
Copy link
Member

ixti commented Dec 19, 2015

Agree!

@tarcieri
Copy link
Member Author

Ok, I just merged #278 which updates the Request API (thanks @ixti) so I guess that means we're going with a breaking API change here

@tarcieri
Copy link
Member Author

I released 1.0.0.pre4 with the updated HTTP::Request API.

@ixti want to take a crack at fixing Webmock?

@ixti
Copy link
Member

ixti commented Dec 19, 2015

Yeah. Sure. Will prepare a PR for webmock today. Sure.

@ixti
Copy link
Member

ixti commented Dec 22, 2015

PR for webmock is: bblimke/webmock#554
Meanwhile, I think we should move webmock integration under http gem.
So that in WebMock there will be only one line left: require "http/webmock".

That will make webmock adapter always up to date with http API.

@tarcieri
Copy link
Member Author

In theory this should be the last breaking change because the 1.0 API should be stable, so I'm not sure that matters?

Perhaps we should move it the next time we feel the need to change it.

@ixti
Copy link
Member

ixti commented Dec 22, 2015

Agree!

y-yagi added a commit to y-yagi/travel_base that referenced this issue Dec 26, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants