Proposed changes for the formatter API #944

Closed
JonRowe opened this Issue Jun 13, 2013 · 10 comments

Projects

None yet

4 participants

@JonRowe
Member
JonRowe commented Jun 13, 2013

The current api for notifying formatters of test suite progress has proved to
be a little inflexible when it comes to adding in new notifications and changing
existing notifications. So we are proposing to change the formatter API in two ways.

Firstly all formatters will be required to tell the reporter which notifications they subscribe to; meaning you can implement the minimum possible to suit your needs.

Secondly all notifications will be a method call with a value object, rather than a string of arguments so that we can change the data passed to notifications without breaking the method signature in future.

So in the future a simple summary formatter might look something like this:

class SummaryFormatter

  def implements
    [:notify_summary]
  end

  def notify_summary(summary)
    puts "Took #{summary.duration} for #{summary.example_count} examples"
  end
end

I'm not certain on the registration format I'd like to use, we already do a simplified version of the above by checking what methods formatters respond to and manually registering, but I'd like to open it up and potentially even allow extensions to send their own notifications which formatters can listen for, but wouldn't be a part of the core api. It might also make more sense to do something like this instead:

class SummaryFormatter

  def register(reporter)
     reporter.subscribe self, :notify_summary, :my_summary
  end

  def my_summary(summary)
    puts "Took #{summary.duration} for #{summary.example_count} examples"
  end
end

In order to allow formatters to have their own internal language and to interrogate the reporter rather than just dumbly report what they implement, but I'm set on using a value object for messages rather than an argument list.

Thoughts? Feedback?

@JonRowe JonRowe was assigned Jun 13, 2013
@xaviershay
Member

Why is the explicit registration better than just checking respond_to?? Or providing a NullFormatter that can be mixed in and selectively overridden.

+1 on data object.

@JonRowe
Member
JonRowe commented Sep 23, 2013

Why is the explicit registration better than just checking respond_to?

Tell don't ask.
No conditional. (Which will be ostensibly a minor performance increases as the cost moves to a single hit up front)

Or providing a NullFormatter that can be mixed in and selectively overridden.

I don't like that use of inheritance, in strictly my opinion it's no better than requiring a 'Base' class and it's unnecessarily ugly.

@myronmarston
Member

Why is the explicit registration better than just checking respond_to?

We've run into some problems with this -- see #966. The TL;DR is that ruby-debug adds start to every object, which is one of the formatter notifications, but ruby-debug's start has a different signature...so when the debugger is loaded, it responded to the method but got ArgumentErrors when the reporter called it.

@JonRowe
Member
JonRowe commented Sep 23, 2013

I'd forgotten about that :)

@xaviershay
Member

I don't like that use of inheritance

Having a null implementation that you build on top of seems a great use of mixins to me, and certainly less complicated then explicit registration, but I guess it's aethestics.

@JonRowe
Member
JonRowe commented Sep 23, 2013

In my eye's it's not less complicated because you have to be aware of the entire api in case you accidentally override something, and because any formatter that does something is by definition not a null formatter, mixing it in feels wrong... I just don't like the use of inheritance for this sort of thing when all you need to do instead is a one shot method call to tell the reporter what you're listening to. This way we're being explicit.

We still intend to allow "legacy" style custom formatters via an adaptor.

@soulcutter
Member

I like the idea of the registration method - potentially it could take an array and/or a hash.

reporter.subscribe self, 
  :start, # sends a message with the same name as the event
  :example_started,
  example_passed: :example_executed, # event: :message_to_send
  example_failed: :example_executed
@JonRowe
Member
JonRowe commented Oct 4, 2013

I don't want to have a duality there, if we want to support differing names I'd rather have two methods, subscribe and subscribe_as

@myronmarston
Member

I believe these changes have been implemented. Closing. @JonRowe, reopen if I'm wrong.

@JonRowe
Member
JonRowe commented Mar 24, 2014

👍

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