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
Formatter registration #1027
Formatter registration #1027
Conversation
Thanks for getting this started! |
Hey, I finally got around to completing this. WDYT @myronmarston @soulcutter ? |
I've added a This is part of #944 (for referencing). |
@@ -51,4 +51,115 @@ | |||
# @see RSpec::Core::Formatters::BaseTextFormatter | |||
# @see RSpec::Core::Reporter | |||
module RSpec::Core::Formatters | |||
|
|||
class Collection |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 This is defintitely a good abstraction -- thanks for taking the time to pull it out. It needs some YARD doc comments though -- could be as simple as # @api private
(I think this should not be part of the public API).
Looking good so far. I have to go get ready for work. I'll try to circle around on the rest of the review later tonight or tomorrow. |
@@ -45,7 +45,7 @@ Feature: read command line configuration options from files | |||
"""ruby | |||
describe "formatter set in custom options file" do | |||
it "sets formatter" do | |||
expect(RSpec.configuration.formatters.first). | |||
expect(RSpec.configuration.formatters.to_a.first). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see why the to_a
is necessary with your current implementation. However, I think it would be better to include Enumerable
in the FormattersCollection
class (and define an each
to delegate to the wrapped formatters
array) -- then no to_a
would be needed here and it would preserve the existing enumerable interface of RSpec.configuration.formatters
. (I suspect some users may rely on it being an array-like/enumerable object as well).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I didn't really want to end up in another metadata
situation where we have to support the full enumerable interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
include Enumerable; def each; end
is all that's needed to support the full enumerable interface.
That said, I think there's a better solution I just thought of. The majority of the code in Formatters::Collection
has nothing to do with the collection of formatters. It has to do with loading formatters. So, what do you think of this?
- Rename it to
Formatters::Loader
. - Remove the collection-style methods from it (
empty?
,clear
,to_a
), but retainformatters
, which returns the formatters it has loaded. - Keep an instance of
Formatters::Loader
inRSpec.configuration
calledformatter_loader
. - Change
RSpec::Core::Configuration#formatters
to returnformatter_loader.formatters
.
Then RSpec.configuration.formatters
will just be an array like it's always been. Your existing Formatters::Collection
class gets paired down to a primary responsibility of loading formatters rather than having a secondary responsibility of presenting a collection interface (which is an SRP violation, I think). No APIs break :).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
I think I've addressed the last few concerns... |
I left one comment about your last refactoring -- but looks good otherwise. Merge away when you're ready. |
Woohoo! Now onwards with part 2... Expect to see something on that front over the weekend. |
Reworking formatter registration, this moves the configuration of formatters out of
RSpec.configuration
into a new objectRSpec::Formatters::Collection
and now all formatters are registered as listeners according to their interface specification.This is step 1 in my proposed refactoring for formatters. Step 2 is converting all the notifications into value objects (I've made a start on this) so we can then extract common logic about notifications into those rather than having massive base classes.
I still need to work on the LegacyFormatter, but I wanted to get some review on this whilst I do that.