Client dependency injection + Chainable is not chainable #7
Conversation
Chainable and Client interfaces were almost the same and their differences seem purely accidental. Having the same interface everywhere seem easier and will allow client injection later.
The Client class already handles Net::HTTP bridging and most of the semantics of the options. It makes sense to frees it from also having to provide http helpers, which are available through Chainable. Removing the helper methods does not hurt (provided Client is part of the private API?). Client#request become *the method* for which injection will take place later.
The :response option handling is already spread between Chainable and Client. It seems it makes sense to avoid unnecessary secret spreading. By the way, a Response object is also returned on HEAD requests made through Chainable#request directly.
|
Another plan to fix the Chainable stuff could be as follows:
This plan is probably cleaner that mimicing Rack middlewares (?) |
The Options class encapsulates the management of options. For now, it implements the following ones: :response, :form, :headers, :before, :after.
No Hash-like write-support is added so far, but it could be later.
The merge operator takes care of merging :callbacks and :headers in a POLS way. For now, :form is not merged as a Hash and the value at right is always used.
Previously Chainable implemented a different heuristics than Client
for :response. Chainable used :object on HEAD and :parsed_body on
other verbs. Client always used :object.
This commit removes this difference and introduces :auto as option
value. It is the default one, it is implemented in Client directly
and it corresponds to the Chainable behavior above. This means that
{:response => :object} is necessary on Client to get its previous
behavior (broken private API, therefore).
By the way, we could restore the previous behavior simply by setting
:object as default in Options and forcing something else through
default_options in Chainable and/or Http. It seemed simpler to get the
auto behavior immediately in Client, though.
This commit fixes the chainable behavior. It works by simply instantiating a Client instance and branching on default options. Chainable#default_headers and default_callbacks will be restored in a future patch. default_client has been removed, as monkey patching branch allows achieving the same goal as Client injection before.
Parameters and EventCallback are no longer used. This commit also restore the default_headers and default_callbacks accessors. The presence of the writers is much arguable IMHO.
|
I've added a couple of commits that fix the chainable stuff. This makes the patch quite big to review, sorry. The commit comments should help. Otherwise, have a look at the final result, the code is quite clear now. The whole stuff occurs in the Options class. |
|
Looks nice, what you did with Options looks pretty close to what I was thinking (removal of EventCallbacks class, using just the one class to store configuration of the chain up to that point). It also should fix the bug that was in my branch, which caused custom headers added by a #with before #on to be lost. |
|
Hi there, sorry I haven't responded to this yet, I've kind of been going balls to the wall on nio4r and Celluloid::IO. I'll look over this and let you know. One thing I did notice was d0a80c6. I would like to make the options hash the default means of dependency injection (which generally follows the whole idea of the options hash as the universal abstraction), not that there's anything suspect about that commit per se. I'd like for people to be able to specify :socket => TCPSocket, as I'd like for people to be able to specify :socket => Celluloid::IO::TCPSocket to be able to do "evented" HTTP client stuff. That will involve a rewrite of the client library with http_parser though. |
Client dependency injection + Chainable is not chainable
|
Overall this looks pretty good, so I'm merging it |
|
Thanks for merging this and for adding me to the committers. I'll probably use pull requests in the future though, at least for possibly controversial features and because they provide a good place for discussing possible directions. Not having an HTTP::Request abstraction prevents us from properly releasing 0.2.0 (say). I'll try to take some time to get into it. |
|
Ideally what I'd like to have is an Http::Request that can be shared between client and server, effectively just data objects, and abstracting the concerns for creating these objects elsewhere (the server I'm writing has a RequestParser that builds the actual Request object). I'd like to factor the server code I'm writing into the Http library as well so it can be a single unified client/server library built on top of http_parser.rb: https://github.com/tmm1/http_parser.rb I'd like for the server components to be an extraction from a quasi-real-world server though. You're welcome to spike out Http::Request though, just be prepared for some changed to it when I merge in the server components. |
Hi!
My aim is here is to allow injection the Client class to use in an easy way when including the Chainable module.
The reason of each commit is explained in the git comment.
By the way, the last commit shows that the Chainable module is not really chainable as implemented so far. I don't know how to fix this exactly. One idea that comes in mind is to mimic Rack with a chain of middlewares responding to
request(verb,uri,options). I've implemented that idea in an experimental branch in my repo, but I'm not really happy with it so far.Any idea?