Support for assigning a stream to SSHKit.config.output is broken

[robd]

Hey @leehambley

I wanted to discuss this before I try to fix it, but I think this syntax, which is included in the Readme, is a bit broken. Do you know, is this supposed to work, or is the readme out of date?

SSHKit.config.output = $stdout

I rigged up a demo script with sshkit 1.7.1 to see what the output actually looks like:

require 'rubygems'
require 'bundler/setup'
require 'sshkit'
require 'sshkit/dsl'

SSHKit.config.output = $stdout

on(:local) do
  execute 'ls -la'
end

The output to this looks like this:

/usr/bin/env ls -la/usr/bin/env ls -la/usr/bin/env ls -la/usr/bin/env ls -la/usr/bin/env ls -la/usr/bin/env ls -la/usr/bin/env ls -la/usr/bin/env ls -la/usr/bin/env ls -la/usr/bin/env ls -la%

The reason for this is a bit involved, but basically has to do with the fact that output << cmd is called for each stdout line, but in this case output is $stdin rather than an implementation of the Abstract formatter. $stdin just calls to_s on the command each time, and the to_s evaluates to /usr/bin/env ls -la.

Since this is broken, do you still want to keep support for this syntax? I have a PR in progress to fix this which wraps the specified output in a Formatter if it's not a SSHKit::Formatter::Abstract (see below), but maybe, since this is broken and no-one has complained we can just drop support for it. People can always do this:

SSHKit.config.output = SomeFormatter.new($stdout)

which is covered in the examples.

If we want to keep support for this, we could just format the stream with the SimpleTextFormatter

def output=(new_output)
  new_output = SSHKit::Formatter::SimpleText.new(new_output) unless new_output.kind_of?(SSHKit::Formatter::Abstract)
  @output = new_output
end

In any case I have a PR which cleans up the tests and exposes this issue.

Cheers, Rob

[leehambley]

I think it's just broken Rob, that was one of the earlier parts of the code that I wrote, the intention was that you set the output to anything that quacks like an IO, but apparently we've broken that somewhere along the line. I think most people drop in a heavier formatter, so I see the value in making the simple case work, but it's also a bit magic, so we could just remove that misleading line from the docs.

[robd]

OK, I will update the docs and remove support for this since it's not that straightforward to implement properly. I started adding support for a Raw Formatter, but this is quite involved, especially to unit test properly.

People can always pass an IO like this which isn't a huge amount of boiler-plate.

SSHKit.config.output = SSHKit::Formatter::SimpleText.new(any_io)

I will pick this up in a PR

[leehambley]

Thanks @robd I don't know what changed in your life that you have such an abundance of free time, but I'm grateful!

[robd]

No probs! My client has delayed our next project and since I have built up some understanding of SSHKit I thought I'd try and help out a bit if I can.

[leehambley]

:) well, the plan's working!