Expose out and err for commands

[aaronmallen]

Introduce @stderr and @stdout instance variables to make error and output streams accessible within commands. This enables more flexible error handling and message output customization.

[aaronmallen]

The idea here is to not have to change the signature of Command#initialize causing a breaking change

[alassek]

How about initializing a pair of StringIO in the command, and then

def perform_command(arguments)
  command, args = parse(kommand, arguments, [])
  command.call(**args)
  IO.copy_stream(command.out, out)
  IO.copy_stream(command.err, err)
end

This would make testing the stdout and stderr of commands much simpler.

[aaronmallen]

The intention is to utilize the out/err passed to the CLI in the first place. dry-cil uses them to print help and errors and "inline" commands. We should also allow the commands to use the user specified out/err. Testing is already trivial as the values are DI

[timriley]

My belated thoughts on this idea. I think it's kind of cool that we can use copy_stream to allow for a dedicated stdout/stderr per-command, but I think this would still surprise a user. We accept injected stdout/stderr objects from the top-level CLI, and if we are going to expose things of the same name inside commands, then I think the expectation would be that they're the same objects.

[timriley]

@aaronmallen I like the idea of this. We already do similar in Hanami CLI, which I think represents good and typical usage of dry-cli:

# in hanami/cli/command.rb

      def initialize(out:, err:, fs:, inflector:)
        super()
        @out = out
        @err = err
        @fs = fs
        @inflector = inflector
      end

I'd love to pull this down into the core library so it's there for everyone who uses it. Since we already make out and err injectable via Dry::CLI#call (good for testing), I think it only makes sense to thread those through to the commands it invoked, and begin setting the expectation that the commands themselves use the provided IO streams, and not manage their own (unless they have good reason to, and in that case, they know they're deviating from the defaults).

In your change here you're conscious to avoid API breakage, but I wonder, perhaps this is worth breaking API for? This feels like a valuable enough thing to move to 2.0 for. And if we did that, is there anything else you'd like to see change with the command interface?

[aaronmallen]

And if we did that, is there anything else you'd like to see change with the command interface?

@timriley The only other thing I modify about Dry::CLI locally would actually be a small breaking change. I typically change the example interface to behave like this:

class MyCommand < Dry::CLI::Command
  example "--with-option", "Do a thing with an option"
  example "--without-option", "Do a thing without an option"
end

via

def example(example, description)
  @examples += [example, description].freeze
end

I then implement:

def examples
  indent = @examples.map { |example, _| example.length }.max + 5
  @examples.map { |example, description| "#{example.ljust(indent)}# #{description}" }
end

The idea being I always have a aligned example/description print out:

Examples:

foo --with-option        # Do a thing with an option
foo --without-option     # Do a thing without an option

[aaronmallen]

I would even go so far as to say, it would be cool if Dry::CLI::Banner took the width of everything into account and aligned everything in a similar fashion.

[timriley]

@aaronmallen This change looks good to me.

I'd like to merge this and then release it as v1.4.0.

Then we'll use #151 as the beginnings of 2.0, which can also include #152.

How does that sound to you? This will probably mean you need to adjust #151 so it's based off this change, rather than off the current main. Would you mind doing that? Thank you!

I'll open a convo with our maintainers just to raise awareness of this change.

[timriley]

Before I merge this, I want to test it with hanami and hanami-cli just to see if there's any breakage with our current internal approach of setting up stderr/stdout.

[timriley]

@aaronmallen Actually, one change request: can you please rename these to @out and @err?

These are the names of the args we currently expose to Dry::CLI#call, so it's good to be consistent. I also think it's better we don't use the literal stdout/stderr names, since these won't always be the actual stdout/stderr for the current process. While it's true that $stderr/$stdout are the default values, the intention is for these to be user replaceable.

[aaronmallen]

@aaronmallen Actually, one change request: can you please rename these to @out and @err?

These are the names of the args we currently expose to Dry::CLI#call, so it's good to be consistent. I also think it's better we don't use the literal stdout/stderr names, since these won't always be the actual stdout/stderr for the current process. While it's true that $stderr/$stdout are the default values, the intention is for these to be user replaceable.

✅ Done @timriley

[timriley]

@aaronmallen Thanks for tweaking the names. FYI, I just pushed two changes:

1. Don't override @out and @err ivars if they're already defined (trying to be as cautious as possible not to break any existing usage for this 1.x version of the feature)
2. Added a test

[timriley]

Will merge this now. Whatever is in the main branch from this merge we'll use as the basis for the next 1.x release. Future commits to main we can use for 2.0.