Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Possible rename of start_dump? Or an additional method. #892

Closed
envygeeks opened this Issue · 18 comments

5 participants

@envygeeks

This is a continuation of a discussion on Twitter with @dchelimsky. Earlier today while building a formatter I noticed that start_dump inside of the formater starts after the ... (or in my case the ✓ ✓ ✓ ✓ ✓ ✓) and before the other information (failure locations and example coverage and such.) I found that a bit odd and it threw me off a little bit.

My opinion: I would love it if start_dump() started before the dump (well my assumption of what the dump is -- the ...) and maybe there be an after_dump() that replaces start_dump() so that we can add a whatever we want before and after our customized output.

@dchelimsky
Owner

If dump means all the shit that rspec produces, to which you alluded on twitter (and I agree is the implication), then I'd say let's loose the term entirely and come up w/ naming like run_report and post_run_report or some such - names that indicate the sequence of events. WDYT?

@dchelimsky
Owner

The trick, of course, is that there are formatters outside this project's control that already use these methods, so this is probably a slow change that requires deprecating some things before the next major release.

@envygeeks

I like those names @dchelimsky they give a little more context on what/where they belong to.

@myronmarston
Owner

We plan to start on RSpec 3 soon, so I'm on board with cleaning up formatter method names for rspec 3.

@dchelimsky
Owner

@myronmarston I support this long term, but I'm concerned about the short term. Even if we clean up the names, I'd strongly recommend supporting the old names through the rspec-3 series (with deprecation warnings). It's one thing to remove APIs (at a major release) that developers can address by updating their own apps, but devs who use custom formatters will be hosed if we render existing custom formatters incompatible and the custom formatter devs don't release rspec-3-compatible versions.

@myronmarston
Owner

@dchelimsky -- good point.

@samphippen
Collaborator

I was thinking we could do what we did with backtrace_clean_patterns. Deprecate what we have at the moment, bring the new stuff into existence. Maybe leave the deprecated thing through rspec 3.0 and drop in 4.0?

@JonRowe
Owner

We've already had some discussion about refactoring formatters anyway for 3 and making slight breaking changes... So this wouldn't be out of our way, I'm a little concerned about leaving old ones in deprecated in light of this, is this not something we can deprecate now / in 2.99?

@dchelimsky
Owner

@JonRowe do you understand my last comment?

@JonRowe
Owner

I believe I do...

If I understand you correctly you're concerned that making breaking changes to the the formatter api in RSpec 3 will unduly affect developers who use external formatters, as they will have to be brought inline before they will work again.

My concern is that this will continue to tie our hands with respect to the formatter api for some considerable time, a major release is exactly the time to make these breaking changes, there are a few open PR's that make breaking changes to this API, which we'd like to merge in 3, but would be unable to do so without breaking these external formatters anyway... or at least not without adding a fair amount of additional complexity to work around them.

@soulcutter and I have both expressed plans for things we would like to do with the formatter and reporter api, namely changing parameters on notifications (I'd like to use a message object rather than arguments), and also adding a more flexible registration system rather than relying on implementing all of the api.

Given that developers can always stop using formatters until they are RSpec compatible, or wait to upgrade until they are, and especially given that 2.99 will be warning them about these problems, I think it's a change we can make now, and not drag out through to 4...

Those are of course, just my 2¢

@dchelimsky
Owner
@JonRowe
Owner

I'd prefer that route as it gives us a fixed time line to make the change over (as opposed to a 3 -> 4 scenario which could be a long time...)

@myronmarston

Bump. @JonRowe, what are your plans for renaming the formatter notifications?

We should decide which if any will be renamed, or close this otherwise.

@myronmarston myronmarston added this to the 3.0 milestone
@JonRowe
Owner

I'd forgotten about this, I'll add it to the list.

@JonRowe
Owner

I'm not sure I care about renaming them... WDYT @myronmarston @xaviershay @dchelimsky

@dchelimsky
Owner

Nothing has changed in terms of my opinions expressed above.

@myronmarston

I'm of two minds here.

On the one hand, RSpec 3, as the first major release of RSpec since 2010, is a unique opportunity to clean things up -- including renaming things that are misnamed. My general philosophy for RSpec 3 has been "let's make all worthwhile breaking changes now" -- which would include a rename of a formatter notification.

OTOH, start_dump, to my mind, has exactly the connotation of what event it represents: the start of the end-of-run dump of the failure details and summary. @envygeeks mentioned surprise that it isn't called before dots are printed (presumably speaking of the progress formatter), but I would have been surprised if that was the case: that's not what the name start_dump suggests to me at all. I know I'm not a good litmus test for that kind of thing (given I'm RSpec's lead maintainer these days and know more about the current state of the rspec code base than just about anyone), but I've never written a custom formatter or worked with the formatter API and had to actually look at the code to confirm that it aligned with what I expected. To my knowledge, @envygeeks is the only one to ever express surprise about this, and even though RSpec 3 is a unique opportunity to change the name, doing so has a cost for others besides us: every custom formatter that uses this event would have to be updated to the new method name. start_dump may not be the perfect name for that event (and a short name like that is certainly open to interpretation) but IMO it's sufficient enough to not warrant the cost of changing it. Also, @JonRowe has done a good job of documenting the formatter interface, and the start_dump notification has docs that explain exactly when it'll get called:

https://github.com/rspec/rspec-core/blob/1e4783cc5e067d1edc17bb9f4537bb85525ac3ab/lib/rspec/core/formatters/base_formatter.rb#L116-L123

...so I'm leaning towards not changing the notification name.

Anyhow, reading what @envygeeks asked for above, it sounds like he's looking for a different notification that is run before any examples run. I was going to suggest we add that, but it looks like the start notification is called then. @envygeeks, does that meet your needs?

@myronmarston

I'm going to close this for the reasons I stated above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.