Extract legacy formatter code

[xaviershay]

@JonRowe

I don't understand what has to happen here but if there is a way I can assist let me know.

[myronmarston]

During my code audit of rspec-core I noticed some things that I think could be improved as part of the final touchups to formatters for RSpec 3. Figured I'd throw them up here for dicsussion:

• BaseFormatter has a bunch of logic to identify the slow examples or groups for profiling. It would be better to move that into some kind of object formatters can delegate to. Would it fit on one of the notification objects?
• Likewise, I think the logic to find/read the failed line can/should be moved a notification or something else that can be delegated to.
• In general, it would be great to get BaseFormatter to a point where there's no significant advantage to subclassing it (and maybe we can do away with it entirely...). The more custom formatters can do through delegating to the notification objects the better, IMO.
• Likewise, Helpers is just a bag of methods -- for any of them that are only useful for one notification it would be good to move them onto that notification object.

I definitely don't want to bike shed on the formatter stuff and delay the RC release but figured it's at least worth mentioning. I imagine some of this may already be on your radar, @JonRowe.

[myronmarston]

BTW, here are some prior discussions of the legacy formatter support gem:

#1304 (comment)
#1309 (comment)

[yelled3]

@myronmarston I'll be happy to assist

[JonRowe]

@yelled3 no need, this is my pet project ;)

[JonRowe]

BaseFormatter has a bunch of logic to identify the slow examples or groups for profiling. It would be better to move that into some kind of object formatters can delegate to. Would it fit on one of the notification objects?

Hmm, I've actually been thinking about a profiling formatter, I wonder if theres scope to make it independent like deprecations.

Likewise, I think the logic to find/read the failed line can/should be moved a notification or something else that can be delegated to.

I was thinking it could probably be extracted into the notification.

In general, it would be great to get BaseFormatter to a point where there's no significant advantage to subclassing it (and maybe we can do away with it entirely...). The more custom formatters can do through delegating to the notification objects the better, IMO.

This has been one of my goals too

Likewise, Helpers is just a bag of methods -- for any of them that are only useful for one notification it would be good to move them onto that notification object.

Yep

[myronmarston]

BaseFormatter has a bunch of logic to identify the slow examples or groups for profiling. It would be better to move that into some kind of object formatters can delegate to. Would it fit on one of the notification objects?

Hmm, I've actually been thinking about a profiling formatter, I wonder if theres scope to make it independent like deprecations.

This has previously been discussed in #726, FWIW. I'm definitely open to the idea, although I had some (minor) concerns I expressed there about it. What do you see as the advantages to making it a separate formatter?

[JonRowe]

@myronmarston can you set up a repo for the extracted gem?

[myronmarston]

Sure. What should it be called? rspec-legacy_formatter_support? (required as rspec/legacy_formatter_support)?

[JonRowe]

Personally I like rspec-legacy-formatters and then require 'rspec/legacy/formatters' which has symmetry with core.

[myronmarston]

Hmm, I think legacy as a directory is kinda weird, personally. I'm fine with rspec-legacy_formatters (rspec/legacy_formatters) if you like...

[JonRowe]

I think of it as a namespace, the fact it requires a folder is immaterial (to me), and as a namespace I think it makes sense. (we may one day have legacy matchers, or such), but I'd be ok with rspec-legacy_formatters

[myronmarston]

Anyone else have an opinion? My preference is very slight...

/cc @xaviershay @samphippen @soulcutter @alindeman @cupakromer

[soulcutter]

I can see it both ways, but I probably have a minor preference for rspec-legacy_formatters simply because it's less-nested and ruby isn't the nicest with deep namespace nesting (which some may say is an advantage).

[JonRowe]

rspec-legacy-formatters has the advantage of being consistent with -'s, which humans find easier ;)

[soulcutter]

rspec-collection_matchers exists, so it's not as though we're breaking ground mixing - and _ here.

The amount of thought that has gone into this is probably too much :)

[fables-tales]

the _ is for joining words, the - is for separating them. rspec-legacy_formatters makes most sense to me as the legacy formatters are more tightly bound together than the RSpec is.

[myronmarston]

Cool, I'll name it that then.

[myronmarston]

https://github.com/rspec/rspec-legacy_formatters

[JonRowe]

the _ is for joining words, the - is for separating them. rspec-legacy_formatters makes most sense to me as the legacy formatters are more tightly bound together than the RSpec is.

With gem naming they represent the underlying structure, not the words. - is used to denote namespaces and thus folders, and _ is used to represent camelcased constants.

[xaviershay]

paint it green

[JonRowe]

See rspec/rspec-legacy_formatters#1

[myronmarston]

@JonRowe, which of the additional items discussed above do you plan to do?

[JonRowe]

I'm going to focus on this next week

[myronmarston]

Which of those items do you plan to do? Also, would it help you get those done faster if I took over the rspec-legacy_formatters work?

[myronmarston]

Closing this one now that the remaining items have separate issues opened for them.