Skip to content
This repository was archived by the owner on Nov 30, 2024. It is now read-only.

Allow adding a subclass of an already added formatter #2019

Merged
merged 2 commits into from
Jul 10, 2015
Merged

Allow adding a subclass of an already added formatter #2019

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d2b9a06
Allow adding a formatter class that extends from an already added for…
Jul 3, 2015
17fb14e
Updated formatter specs
Jul 9, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/rspec/core/formatters.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ def register(formatter, notifications)

def duplicate_formatter_exists?(new_formatter)
@formatters.any? do |formatter|
formatter.class === new_formatter && formatter.output == new_formatter.output
formatter.class == new_formatter.class && formatter.output == new_formatter.output
end
end

Expand Down
11 changes: 11 additions & 0 deletions spec/rspec/core/formatters_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
require 'pathname'
require 'rspec/core/resources/a_custom_formatter'

module RSpec::Core::Formatters
RSpec.describe Loader do
Expand Down Expand Up @@ -137,6 +138,16 @@ module RSpec::Core::Formatters
}.to change { loader.formatters.length }
end
end

context "When a custom formatter exists" do
before { loader.add Custom::AGeneralFormatter, output }

it "adds a subclass of that formatter for the same output target" do
expect {
loader.add Custom::AMoreSpecificFormatter, output
}.to change { loader.formatters.length }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd prefer this if the classes were declared as local variables / lets inline rather than using ones prepared in another file, especially as for this spec it's completely unnecessary to have an implementation.

e.g.

a_general_formatter = Struct.new(:output)
a_more_specific_formatter = Class.new(a_general_formatter)

before { loader.add a_general_formatter, output }

it "adds a subclass of that formatter for the same output target" do
  expect {
    loader.add a_more_specific_formatter, output
  }.to change { loader.formatters.length }
end

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tried that, but it doesn't work. None of the formatters are added.

and if you do instead

generic_formatter = BaseFormatter
specific_formatter = Class.new(generic_formatter)

Only the generic formatter is added (looks like classes don't get added unless they've registered).

The following works though (and doesn't require the custom formatter classes)

specific_formatter = RSpec::Core::Formatters::JsonFormatter
generic_formatter = specific_formatter.superclass

I'll add those changes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yep, you do need to register otherwise you're considered a legacy formatter, still the new way is better, thanks!

end
end
end

describe "#setup_default" do
Expand Down
22 changes: 22 additions & 0 deletions spec/rspec/core/resources/a_custom_formatter.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
module Custom
class AGeneralFormatter < RSpec::Core::Formatters::BaseFormatter
RSpec::Core::Formatters.register self, :message
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you remove this as above this isn't relevant, but this leaks into the rest of the spec suite and given it's not a real formatter shouldn't really be done, nor is it needed for your specs.

attr_reader :call_times

def message(_notification)
puts message_builder
end

def message_builder
'Important Message'
end
end

class AMoreSpecificFormatter < AGeneralFormatter
RSpec::Core::Formatters.register self, :message
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above.


def message_builder
'Very ' + super
end
end
end