Prevent formater loader registering duplicate listeners #1990
Conversation
JonRowe
force-pushed
the
fix_duplicate_formatters
branch
from
fb323b5 to
87dfa07
Compare
|
Ping @myronmarston |
| @reporter.register_listener formatter, *notifications_for(formatter_class) | ||
| elsif defined?(RSpec::LegacyFormatters) | ||
| formatter = RSpec::LegacyFormatters.load_formatter formatter_class, *args | ||
| return formatter if duplicate_formatter_exists?(formatter) |
There was a problem hiding this comment.
You've got two return statements here but only one spec. Presumably only one of them is tested? Would it be easy to add a test for the other branch?
Alternately (or maybe in addition...), there's duplication here, and it's definitely duplicated knowledge. Instead of return formatter if ...; @reporter.register_listener, WDYT about extracting a private register_listener helper method that would do the duplicate_formatter_exists? check and then call @reporter.register_listener? I'd feel better about having a spec for only one of these branches if they were going through a shared helper method.
JonRowe
force-pushed
the
fix_duplicate_formatters
branch
from
87dfa07 to
1218273
Compare
| @@ -163,7 +163,6 @@ def add(formatter_to_use, *paths) | |||
| WARNING | |||
| return | |||
| end | |||
| @formatters << formatter unless duplicate_formatter_exists?(formatter) | |||
There was a problem hiding this comment.
Now that we no longer mutate @formatter after the if/elseif/else/end block, should we remove the return on line 164?
|
LGTM |
JonRowe
force-pushed
the
fix_duplicate_formatters
branch
from
1218273 to
4455319
Compare
|
Merge when green. |
Prevent formater loader registering duplicate listeners
Prevent formater loader registering duplicate listeners
Fixes #1987