utils/tty: handling a TTY stderr

[nandahkrishna]

• Have you followed the guidelines in our Contributing document?
• Have you checked to ensure there aren't other open Pull Requests for the same change?
• Have you added an explanation of what your changes do and why you'd like us to include them?
• Have you written new tests for your changes? Here's an example.
• Have you successfully run brew style with your changes locally?
• Have you successfully run brew tests with your changes locally?

---

In the current implementation of the Tty module, the result of $stdout.tty? is used to decide whether the output can contain special TTY codes (colours, formatting and so on).

However there are some problems with this, as discussed in #8577:

• Redirection of only stderr to a file: if there are coloured outputs in stderr (such as in onoe), the special characters feature in the file. For example, without a Livecheck Watchlist, brew livecheck 2> test.txt results in text.txt containing colour-code characters.
• Redirection of only stdout: stderr loses colour output.

This PR aims to fix these, and I think using an @output variable (which is $stdout by default and can be changed to $stderr) makes sense. Here are the options I've considered, though I'm not sure all of these would make sense:

• Override $stderr.puts – set @output to $stderr, carry out the normal puts logic, reset @output – maybe not a great idea.
• A new Tty::Error module (current implementation) – only difference is that @output is set to $stderr – I'm not satisfied with it, doesn't seem like a good fix.
• Changing Tty to a class and using an $stdout instance and an $stderr instance as required.

I'm not sure anything else that's crossed my mind is either possible or worth considering. I'd love to hear your comments and suggestions on how we can proceed with this. Thanks!

Redirection examples

1. $stderr to a file
   

   ➜ brew livecheck --tap 2> output.json
   

   output.json

   ^[[31mError:^[[0m missing argument: --tap
   

2. $stdout to a file

   ➜ brew livecheck --tap > output.json 
   Error: missing argument: --tap
   

   Note: `Error` was not coloured red.

[reitermarkus]

Maybe just pass the stream?

def color?(stream = $stdout)
  …
end

[nandahkrishna]

The issue with that is color? is called by to_s (which is in-turn chained at the end of other methods when we're using puts), and I don't think we want to pass a parameter to that? It would require adding a parameter to all methods in Formatter (though it would only be provided for $stderr). Hope I'm not missing something obvious.

[reitermarkus]

It would require adding a parameter to all methods in Formatter

I don't really see how else we can properly detect the output stream. At the point where we are calling either puts or $stderr.puts the escape codes are already interpolated into a string.

[reitermarkus]

We'd need to have some sort of

$stderr.puts do
  # Tty.color? uses $stderr in here.
end

[nandahkrishna]

Yeah, this would be the simplest of all the implementations (I think), but I wasn't sure overriding puts was a good idea. I'll dig deeper into this particular option and see what I get.

[reitermarkus]

It can be a Ruby refinement I think.

[nandahkrishna]

It can be a Ruby refinement I think.

Oh thanks, I'll take a look 👍🏼

[nandahkrishna]

Earlier I tried creating a refinement and added it right at the top of brew.rb (adding it elsewhere didn't seem to work, for some reason). I was too groggy to understand what was going on but now I think I do, to an extent. While my debug statements in the refinement didn't affect puts, they did affect $stderr.puts and $stdout.puts (when I checked using brew irb).

I failed to realise earlier that the String would be interpolated by that time (just as you'd mentioned), so my earlier implementation wasn't correct. I've now created two methods, puts_err and print_err – we pass a block to these methods, which change the Tty output to stderr, yield and then change the output back to stdout. I went ahead and changed most occurrences of $stderr.puts to puts_err (and $stderr.print to print_err).

If this implementation isn't preferred and anyone has comments/suggestions, please do let me know.

[MikeMcQuaid]

Code seems fine but I wonder if we need to use this everywhere rather than just places using colours?

[reitermarkus]

I think what might be a nicer API for this would be:

$stderr.output do
  print "No newline yet: "
  puts "Formatter.url("https://example.com/")
end

This would also work the same way with $stdout.

[reitermarkus]

Something like:

module Kernel
  def output(&block)
    Tty.with(self) do |stream|
      stream.instance_eval(&block)
    end
  end
end

module Tty
 module_function

  def with(stream)
    previous_stream = current_stream
    self.current_stream = stream
   
    yield stream
  ensure
    self.current_stream = previous_stream
  end
end

[nandahkrishna]

I wonder if we need to use this everywhere rather than just places using colours?

@MikeMcQuaid The main reason I changed all occurrences of $stderr.puts (or print) to use this was consistency. I also thought people might be confused about which to use unless they have prior knowledge of this PR or the Tty module, and realise that this is used only when formatting is required.

[nandahkrishna]

With the latest push, this PR has been refactored based on the suggestions from @reitermarkus. Unfortunately I wasn't able to implement it in that exact form, here's why:

$stderr.output do
  puts "Hello"
end
# Works fine

$stderr.output do
  call_a_function_to_print("Hello")
end
# Doesn't work – `$stderr` doesn't have a `call_a_function_to_print` method

The only solution that came to mind (which would retain the simplicity of the suggestion) was to replace puts within this "context" and then restore it afterwards. So that's what I've done here.

I don't entirely understand why exactly some of these method calls need to be prefixed with Kernel. and the others don't (significant bit of experimentation done to arrive at this), I pin that on my lack of in-depth knowledge of Ruby, haha. Simply put, change something and it doesn't work for a variety of reasons, just not able to exactly say why it behaves that way 😅.

If this needs to be changed further or improved, let me know. Thanks!

[MikeMcQuaid]

Looking good so far. Interested in @reitermarkus' thoughts. #8633 would be good to tackle in here too as it's loosely related?

[nandahkrishna]

The changes have been made, all that's left is to include the fix for #8633 here.

[nandahkrishna]

I think everything's done here. Would you mind just checking one last time @reitermarkus? Thanks.

[reitermarkus]

Looking good so far. I think there are a few more instances where currently puts e is used to output exceptions. These should also use this, first of all because they should be in $stderr in any case, and secondly because we have a few exception classes that use Formatter in their #to_s.

[reitermarkus]

I also just saw an instance where opoo is used with Formatter.

[nandahkrishna]

@reitermarkus I believe I've fixed all puts e instances I could find. In addition I ensured that reason for MissingFormula is also handled using this approach. Some errors/debug which were not earlier printed to stderr have now been changed to do so.

It's important to note that I have not used Tty.with unless absolutely necessary, ie, if there is a possibility of Formatter use in the string. So for example,

puts "Error: this is an error"

would be modifed to

$stderr.puts "Error: this is an error"

without Tty.with, as it is unnecessary.

The only thing that I was unsure about was exceptions.rb – do I change all the puts there to $stderr.puts? Also, do let me know if there any other instances of debug/error info printing that I've not changed yet. I did a fair bit of grep-ing but it is possible I've missed a few cases.

[MikeMcQuaid]

Let's strip this back to the cases where it's needed and use $stderr.puts otherwise

[MikeMcQuaid]

The more I think about this PR the more it seems like solving some problems that don't really exist. We've never had any user complaints about e.g. exceptions being outputted incorrectly when redirected stderr. If you're experiencing a problem specifically with livecheck: let's fix this just for Livecheck but in a way that the code can be extracted elsewhere if needed later.

[nandahkrishna]

The problem doesn't exist just for livecheck, if any stderr output that contains formatting is redirected to a file, it still contains ANSI characters though it shouldn't (as in the example output in the original comment). This is due to the Tty module not accounting for stderr. We don't get reports because it isn't common that a command's output is redirected to a file, as far as I know.

While I believe the change to the Tty module should be made, I don't mind discarding the other changes. It bothers me ever so slightly that the issue would persist, but if we'd rather only fix the problem for the case where it's absolutely necessary (livecheck for now), I'll go ahead and reduce this PR to just the Tty module changes.

[MikeMcQuaid]

if we'd rather only fix the problem for the case where it's absolutely necessary (livecheck for now), I'll go ahead and reduce this PR to just the Tty module changes.

Thanks ❤️

[nandahkrishna]

@MikeMcQuaid the PR has been reduced to do just 2 things:

1. Add the ability to handle stderr in Tty.
2. Fix issue Send "Tapping" output to stderr #8633.

Let me know if any other changes are required. Thanks!

[MikeMcQuaid]

LGTM, thanks again, nice work!