Use configuration rather than streams directly in deprecation formatter and fall back to std pipes

[JonRowe]

Potential solution to #1127

[JonRowe]

Note, we either need to do this consistently across the two streams, or rework this into the formatter (due to how we used to pass in the pipes) or make the pipes themselves smarter... (e.g. if we give them a class that takes a default value, but allow them to be reconfigurable by mutation rather than replacement, we could also then ask the pipe if configured in the commandline runner)

[myronmarston]

Thanks for taking a stab at this, @JonRowe. This solves the immediate problem, but it feels like it's working around what is a deeper design issue that we should solve more systemically. Specifically:

• Why do we allow configuration.output_stream to be nil at all? Why not default it to $stdout to begin with?
• Why do we have two systems for configuring output/error streams (configuration and CommandLine#run)? I think we can (and should) simplify to one (but we need to do our homework to see what this might break).
• Checking configuration.output_stream to see if it's before configuration has been allowed to happen is pretty round about. Plus, it could be configured later to a new value. For some other config values, we have the concept of them being "locked" once a certain point is reached because we've had to do things based on those config values and have reached a "point of no return". For example, see this, this, this and this. It might be worth having a similar concept for the stream config because it would be confusing for users to try to change it later and have it ignored.

All that said -- some of my ideas above would be breaking changes that are only appropriate for master/3.0, and what you have here might be the right solution for 2.99. Thoughts?

[myronmarston]

Your commit message made me chuckle :).

What are your thoughts on the stuff I put above? I'm thinking this is a good solution for 2.99 but for 3.0 we may go a different direction.

[JonRowe]

• Why do we allow configuration.output_stream to be nil at all? Why not default it to $stdout to begin with?

It should, we talked about addressing it before but the ||= in runner was an 'expected' behaviour at the time.

• Why do we have two systems for configuring output/error streams (configuration and CommandLine#run)? I think we can (and should) simplify to one (but we need to do our homework to see what this might break).

So you can configure them at runtime programatically and extend if your a 3rd party. I'd like to see runner configuring the streams at runtime (they would then be overridable in spec_helper etc via configuration).

• Checking configuration.output_stream to see if it's before configuration has been allowed to happen is pretty round about. Plus, it could be configured later to a new value. For some other config values, we have the concept of them being "locked" once a certain point is reached because we've had to do things based on those config values and have reached a "point of no return". It might be worth having a similar concept for the stream config because it would be confusing for users to try to change it later and have it ignored.

If we defer to configuration there's no need to lock to any specific value, just the current configuration. I'm mostly thinking about tools which rerun specs or otherwise modify things to which this would advantage. If we don't depend on them being locked it's easier to write such tools.

All that said -- some of my ideas above would be breaking changes that are only appropriate for master/3.0, and what you have here might be the right solution for 2.99. Thoughts?

I'd start with merging a version of this, back porting to 2.99 and then refactoring to suit.

[myronmarston]

It should, we talked about addressing it before but the ||= in runner was an 'expected' behaviour at the time.

IMO, the expected/desired behavior is to make RSpec::Core::Configuration#output_stream= actually take affect. According to the commit message in a6ed0c8, that was the bug being fixed, and using ||= was the smallest change that fixed that bug.

I still think that allowing RSpec::Core::Configuration#output_stream to ever be nil is a bad design, though, which is why I'm interested in fixing this more holistically. On top of that, it looks like error_stream can't be set via configuration because it'll be overriden in that method since it doesn't use ||=.

So you can configure them at runtime programatically and extend if your a 3rd party. I'd like to see runner configuring the streams at runtime (they would then be overridable in spec_helper etc via configuration).

"So you can configure" -- that's why we provide RSpec::Core::Configuration. We have lots of configuration settings that can be configured at runtime programatically and extended by a 3rd party, and none of the others have this complexity/confusion around having two ways to set it and which one wins.

If we defer to configuration there's no need to lock to any specific value, just the current configuration. I'm mostly thinking about tools which rerun specs or otherwise modify things to which this would advantage. If we don't depend on them being locked it's easier to write such tools.

The problem I'm seeing (both with the current code and with the changes in this PR) is that once the @printer instance variable of RSpec::Core::Formatters::DeprecationFormatter gets set, later calls to RSpec::Core::Configuration#output_stream= are no-ops (because the formatter's printer is not subsequently updated to match), but we do not inform the user that that is the case. In affect, that config setting is implicitly locked once the deprecation formatter picks which printer to use; we just don't lock it explicitly and wind up with a confusing situation where calls to #output_stream= silently fail. If there's a simple way to get it to work so that calling #output_stream= at any time works, I'm all ears, but I haven't thought of a way...and locking that config setting once we're not longer going to react to it being changed seems like a reasonable solution.

I'd start with merging a version of this, back porting to 2.99 and then refactoring to suit.

Do you mind backporting this to 2.99 first since I'm still not convinced this is the solution we want for 3.0?

[myronmarston]

This was solved by #1142 instead.