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

Support observers #478

Closed
ixti opened this issue May 28, 2018 · 3 comments
Closed

Support observers #478

ixti opened this issue May 28, 2018 · 3 comments

Comments

@ixti
Copy link
Member

ixti commented May 28, 2018

In #476 I proposed to warn in some edge cases. Truth is - I hate when libraries are polluting STDERR/STDOUT without the way to control their verbosity level. So my first idea was - let's provide logging API, but in fact logger is simply a very specific use case of observer:

class MyHttpLogger < HTTP::Observer
  def initialize(logger)
    @logger = logger
  end

  def request_sent(request:)
    @logger.debug("#{request.verb} #{request.uri}")
  end
end

HTTP.observe(MyHttpLogger.new(Rails.logger)).get(url)

One thing that really bothers me in most Russian doll middleware architecture is pointless overhead of Array#each. With single observer object it will be possible to simply call:

observer&.request_sent(:request => request)

Providing HTTP::Observer base class will allow third parties to inject needed functionality on it using plain old Ruby:

HTTP::Observer.extend HotPotatoExtensions
@tarcieri
Copy link
Member

tarcieri commented May 28, 2018

There's been a ton of discussion about this (and even some merged code). You even have an issue open for it already I think: #209

See also #177 and #240, as well as #346.

I think some sort of hook system is great and needed for things like caching (#223).

It'd also have the potential to simplify the webmock binding (#212).

And yes, logging for debugging purposes would be great (#238, #348, #437). I'd really love something like what I implemented for Reel: https://github.com/celluloid/reel/wiki/Spy

I am definitely a fan of avoiding the "middleware" pattern, especially as it pollutes the stack and makes backtraces that much more annoying to read (not to mention the performance implications).

One thing I'd suggest is avoiding the "null object pattern" for this (i.e. defining a class of observers that does nothing), in lieu of gating the invocations on a branch. Ruby method dispatch is rather slow, and the null object pattern would greatly increase the number of method invocations this library performs.

@ixti
Copy link
Member Author

ixti commented May 28, 2018

I feel myself so dumb now. Indeed we already had this conversation before! o_O Well the idea I had is pretty same to Spy in fact. with exception that there's a default implementation of observer and one will be able to just implement only needed handler to avoid if x.respond_to? guards which will impact performance. But in general API should accept an object that simply responds to all needed methods. Pretty much as Spy in fact. Just with ability to pass in any Spy-compliant implementation :D

@ixti
Copy link
Member Author

ixti commented May 28, 2018

Duplicate of my own issue indeed. :D

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

2 participants