Access the HTTP::Request from the HTTP::Response #546
Conversation
joshuaflanagan
force-pushed
the
response_request
branch
2 times, most recently
from
99ac40f
to
3aae264
Compare
This mirrors httprb#546, which has not been merged yet.
Versions of HTTP.rb that allow access to the Request from the Response (httprb/http#546) require this change to continue working.
Versions of HTTP.rb that allow access to the Request from the Response (httprb/http#546) require this change to continue working.
|
What are the outstanding issues blocking this from being merged? I’ve been running this in production for 6 months now - happy to help get it merged so others can get the improved logging ability. |
|
@joshuaflanagan if you can rebase I will review/merge |
joshuaflanagan
force-pushed
the
response_request
branch
from
3aae264
to
236695b
Compare
|
@tarcieri ok done. The mobile github view made it harder to tell that there were merge conflicts. All fixed now. |
lib/http/request.rb
Outdated
| @@ -199,6 +199,10 @@ def inspect | |||
| "#<#{self.class}/#{@version} #{verb.to_s.upcase} #{uri}>" | |||
| end | |||
|
|
|||
| def self.example | |||
| new(:verb => :get, :uri => "http://example.com") | |||
| end | |||
There was a problem hiding this comment.
Would you mind moving this into the specs themselves so it isn't otherwise defined when the specs aren't running? You can still define it on HTTP::Request
There was a problem hiding this comment.
I'm ok with removing the example from the Request definition, but don't think it is worth re-opening the class in the specs. I think that would be more confusing to readers to see the class method invoked, but not defined where you would expect to find it.
I think the specs will be more readable to just repeat new(:verb => :get, :uri => "http://example.com") where needed. I can make that change if you agree.
This fixes httprb#463 This will greatly enhance the abilities of pluggable `Features` by being able to reference the Request that initiated a Response.
During normal operation, the Response#uri will be pulled from the Request passed into the Response. However, we still allow explicitly setting a `:uri` option when creating a Response, for backwards compatibility. This was motivated by rubocop/code climate complaining that `Client#perform` was now 1-line too long. I'll defer to the project maintainers to decide if that is a good thing.
joshuaflanagan
force-pushed
the
response_request
branch
from
236695b
to
57ecccc
Compare
|
Thank you! Sorry about the long delay |
Make them closer to reality. Related to: #546
* Use Request#uri in Response Response initialize requires Request object now, making uri parsing absolutely redundant. And as it's a breaking change anyway - we should get rid of `:uri` option completely. NOTE: We need to update WebMock's adapter to reflect this change prior releasing v5.0.0 * Update Changelog with #546 changes SEE ALSO -------- ShippingEasy/webmock@505ad00
|
Hi, this is listed as a breaking change at https://github.com/httprb/http/blob/master/CHANGES.md (presumably for 5.0.0.pre and an upcoming 5.0.0?) From the PR description and diff, it's not yet clear to me what the nature of the breaking change is. Can you provide more guidance? Is the backwards incompat only if you are manually instantiating an HTTP::Response, now it takes different args? (I'd imagine manually instantiating an HTTP::Response is relatively uncommon) Thanks! |
|
@jrochkind previously the request URI was a directly accessible member of the response object. That was removed. However, now the original request is a directly accessible member object of the response, and the request object knows the original URI. So, it removes direct access to the request URI, but it's still available via another layer of indirection through the request. |
|
We have This is a breaking change for integrators like WebMock for example. |
This fixes #463
This will greatly enhance the abilities of pluggable
Featuresbybeing able to reference the Request that initiated a Response. It will make it easy to solves issues like #545.
I wasn't very happy with all the changes to the specs to put a "fake" Request, but felt it was pretty important that the Request object be mandatory when creating a Response. If we left it optional, it might be too easy to create a new code path (like the
auto_inflatecode) that doesn't properly set the initiating Request. Open to suggestions.