grab bag of feedback

[gurgeous]

This is neat! Some quick thoughts after a few minutes of poking around. I really like it, so please don't take any of this feedback as negative. It's much easier to list the stuff that stood out rather than comment on the overall package, which is excellent. Thanks for putting this together!

• That output is awesome. Any reason to include the random # in the line prefix? I'd think the pid would be enough and it would be great to reduce noise.
• Is this using a shell, or running the process directly? This makes a big difference when writing commands and can often result in unexpected behavior, especially when trying to use redirects or pipes. Which is quite common in my experience. :)
• Actually, that touches on a larger issue. I think it should be possible to just do: cmd.execute("sudo apt-get install xyz > /tmp/output.txt") instead of: cmd.execute(:sudo, "apt-get", "install", ...) This is the most common use case, in my opinion, and supporting this pattern will encourage people to stop using bash to write this stuff.
• Escaping can be a challenge with the current syntax. What should this do? cmd.execute(:ls, "hello world")

[71555-047a57b2] Running ls hello world
[71555-047a57b2]    ls: hello: No such file or directory
[71555-047a57b2]    ls: world: No such file or directory
[71555-047a57b2] Finished in 0.002 seconds with exit status 1 (failed)

• Any reason to use symbols vs. strings for the command name? Sadly some commands have spaces in them...
• I think that commands should throw exceptions if they fail by default. This is one of the biggest design flaws in bash, and I can't tell you the number of problems it's caused me (and people who work with me) over the past decade. When I write scripts I almost always want to halt on failure. I recognize that if we make this change we can't use execute!, sadly.
• Timeout looks really handy. I've definitely wanted this a few times.
• I think you could probably ditch the with exit status 0 (successful) and only log failures. That will make them stand out more, I think. Or at least make successful use the same color as the rest of the line.
• Features I think you can safely ditch: -- env: (this can easily be handled with ENV) -- chdir (easily handled with Dir.chdir("x") { ... } -- redirection (typically in the command, not needed in ruby land) -- :user/:group/:umask (that's what sudo is for)
• Can I convince you to change execute to run? I love brevity and there will never be a chance to change it after this! :)

Here's a typical example, one of hundreds that I have from my old startup. Think about this snippet - would it make sense to separate out every argument? Would it make sense for the script to continue after failure? Would it make sense to replace shell redirects/pipes with a custom syntax? Just some food for thought.

  # restore db
  def db
    if Util.mac?
      chmod, dst_dir = "#{user}:admin", "/usr/local/var/mongodb"
    else
      chmod, dst_dir = "mongodb", "/var/lib/mongodb"
    end

    Util.rm_and_mkdir(TMP)
    Dir.chdir(TMP) do
      # untar
      banner "Untarring #{db_src}..."
      run("tar xvfz #{db_src} --exclude 'prealloc*'")

      # atomically move the files into place
      with_db_stopped do
        Dir.chdir("mongodb") do
          Dir["startup*"].sort.each do |src|
            next if src == "lost+found"
            next if src =~ /\.old$/

            dst = "#{dst_dir}/#{src}"
            old = "#{dst}.old"
            run "sudo rm -rf #{old}"
            run "sudo mv #{dst} #{old}" if File.exist?(dst)
            run "sudo chown #{chmod} #{src}"
            run "sudo mv #{src} #{dst}"
            run "sudo restore > /tmp/restore.txt"
          end
        end
      end
    end
  end

Happy to help implement any/all changes... Thanks again!

[piotrmurach]

Thanks, that's great feedback!

Few initial thoughts:

• I agree the uuid should probably be shorter. I have followed the same principle that is used for creating tempfile names in ruby, though I'm not sure that PID is the one I will choose as you can run many commands and they all may run in the same process but it would be good to have unique identifiers per command executed. Also, I want to provide ability to specify different printers apart from the current one such that skip printing the uuid all together and still colour the output.
• Basically the execute relies on spawn similarly to exec, system and open3 Ruby commands. This means execution in new process environment that isolates the command from side effects. Which also gets all the environment variables and options passed down.
• Sure you can run the command the way you have mentioned. The readme is definitely not comprehensive:) If you run cmd.execute("ls -la > output") it will respect the redirect and execute directly the string. There are many ways to pass the arguments which try to mimic the spawn behaviour.
• You can use both symbol & string, makes no difference. The reason why I provided the examples this way is simply a stage setup for more ruby like syntax. I was intending to add ability to basically run cmd.ls "-la" and the command just do what is intended. Agree it won't work in a lot of cases given complex arguments or executables but it is a nice Ruby-like way to run simple stuff if someone needs to.
• I think designing api we need to be careful assuming too much. I completely understand the deficiency of bash but also believe in providing the means for handling the rough edges to the user. Raising error is not always the best way to handle application logic flow. I can easily see circumstance where a person will want to examine exit status and then decide whether raising error is what's needed or maybe simply aborting script with some useful debug info. I find raising errors for a generic library too rigid. Hence, I would still think of having dangerous version of the command running. Best practice can be explained in readme and ways to handle it?
• I'm not sure about ditching the features. I think constructing a uniform commands out of options and properly escaping these is quite important. For example :group option is tricky to escape as part of larger command and if the tested code can do it for you so much better. The :env does quite few things for you including transformation of symbols into proper environment variables. Again, I think if the library can provide value in constructing these I think it should, especially when user can focus on writing Ruby?
• I'm not dead set on execute vs run, we can defo come back to this point once we clear up all the other issues.

[piotrmurach]

On another note, it would be great to open up separate issues for each bullet point. Then we are able to reference commits against features. I should be able to find some time tomorrow.

[piotrmurach]

I've moved all the feedback to separate issues.