Output streaming erroneously disabled when hide is in play

[bitprophet]

Bewildered this hasn't come up before, but a co-worker was trying to make heavy use of out_stream and found that hide turns off streaming entirely, vs my recollection that eg hide='stdout' or hide=True only turn off the default shoveling to sys.stdout.

However the code is too naive/"pure":

• The core machinery is Runner._handle_output.
• It takes a buffer, a bool-for-that-one-stream hide arg, the output stream, etc.
• The bits making that output stream default to sys.stdout, etc, are higher up - this method doesn't currently know or care how that is determined.
• If hide indicates this stream should be hidden, the step of self.write_our_output() to the output stream is skipped.
  • As you can guess from the previous bullet point, there's no further logic here - this method does not care whether output was the default-to-your-terminal stream or not.

Obvious solutions:

• Make 'hide' not applicable to custom stream arguments - this is the absolute most obvious approach and is how I assumed things worked until I double checked. Somehow - either within _handle_output or higher up and add a new kwarg to the former - test whether the user has actually overridden out_stream/err_stream, and ignore hide in that case.
  • Pros: presumably intuitive to most users
  • Cons: leans into 'magic', is arguably backwards incompatible (though I cannot really see somebody RELYING on the current behavior as it basically kills your ability to completely redirect output somewhere else...)
• Leave as-is, but document that folks should use the watcher functionality instead. In other words, embrace the current "hiding applies to the output streams regardless" behavior and state that you should use a watcher to 'shovel' data elsewhere.
  • Pros: it's a doc fix only so less effort / less breakage
  • Cons: this seems incredibly dumb -- especially given that out_stream/err_stream have always been pitched/intended as "the way you would make stuff go somewhere besides the user's terminal", with watchers coming later and being more of a "stuff you might want to have happen in addition to streaming output to a terminal or log" feature, such as the prompt autoresponse it was built for.

[bitprophet]

Musings on the right way to implement that first solution:

• Explicitly check for whether out_stream is sys.stdout
  • Pro: relatively bulletproof re: the intent of "only hide if it's going to the user's terminal"
  • Con: if the user's session has replaced sys.stdout with something else (eg test harness or other headless capturing approaches) we'll still hide.
    • I think this is correct still, since most of the time when somebody is manually reassigning sys.stdout, they are expecting the code being wrapped to act 'normally'. Us trying to second-guess would probably be net negative.
  • Con: arguably, this approach's test is simply a worse, more roundabout way of testing the check below re: whether we supplied the fallback stream value. If that's truly what we want to test, let's test that?
• Only check whether the user supplied a value for out_stream; eg if done in _run_opts, this would be testing for ANY non-default value, whether kwarg or config-derived. This is the spot where we throw in sys.stdout when the user didn't give anything.
  • Pro: is more explicitly testing for the "you surely don't want to both redirect a stream and hide it at the same time, which you are definitely asking me to do here" scenario.
  • Con: I could maybe see some corner cases where a user is overriding these objects but still wants them subject to hiding for whatever reason (eg they're somehow going to another real terminal device, and this is being done via out_stream and not the surrounding shell environment because...reasons...?
    • this seems pretty specious and unlikely...
  • Con: The mechanics of coding this up would be somewhat more complex (requiring more values passed around such that _handle_output can tell whether the stream it has is the default one).

[bitprophet]

Given how we've structured the code so far, option 1 there isn't actually trivial anyways, because _handle_output doesn't want to know which stream it is dealing with (IOW it doesn't know if it's stdout or stderr focused). So I'm going to try option 2, which feels more correct anyways.

[bitprophet]

I think this can be considered a bugfix (despite the ever so small chance of backwards incompat breakage) so actually moving that to base on the 1.0 branch...

[bitprophet]

Ran into this as part of #682 too (insofar as the new asynchronous option also impacts hiding streams) and tackling it in that branch. Independently had settled on option 2 from above mental spew (only look at whether the argument had some non-default/None value, instead of the above commit's option 1 approach of "is it sys.stdout/err"), so that's good confirmation.