Refactoring

[njam]

I like the idea of having a simple API to run commands and retrieve their output and exit status.
For my use cases I wanted to add some features (like combined stdout+stderr, allowing to pass stdin, etc).
I thought it would be good to refactor the code a bit first to allow to add these features more easily.

• I separated the code into three classes:
  • Definition: Holds information about a command that should be run (only cmd string atm)
  • Result: Holds information about the result of a command (stdout, exitstatus etc)
  • Runner: Takes a Definition, runs the command and returns the Result
• I changed the specs to run actual commands instead of mocking open3. I thought this will make it easier to change the code that runs the commands in the future. Now the Runner test is more like an integration test.
• The documented public API is the same as it was

@laserlemon What do you think?

[coveralls]

Changes Unknown when pulling 93e701c on njam:refactor into * on collectiveidea:master*.

[laserlemon]

@njam Thank you! 👏 I appreciate you taking the time to contribute. I like how you changed the specs to be more full stack. I also have a couple of minor concerns and one major one…

Minor

• We prefer double quotes but more importantly, I don't think that a switch in either direction belongs in a structural refactor. It muddies the waters.
• I'm unsure of the usefulness of the Command::Definition class since all it does is hold a single string. It feels premature.

Major

• I'm not convinced of Command's future usefulness. It was initially intended as a simpler alternative to Open3 that would eliminate the need to google for the API every time. However, there's something that Open3 can do that Command cannot. I would love to be able to do this:

  command = Command.run("do-something")
  command.stdout # => "good\nugly\n"
  command.stderr # => "bad\n"
  command.output # => "good\nbad\nugly\n"

  There's no way (that I can find) to make Command reliably combine the stdout and stderr into one output in such a way that we can guarantee its sequence. I can make tests pass along these lines intermittently at best. I believe that that's the killer feature, so short of that, I'm not so sure about Command's future.

[coveralls]

Changes Unknown when pulling d5b08ab on njam:refactor into * on collectiveidea:master*.

[njam]

We prefer double quotes

Well, I tried ;)

I'm unsure of the usefulness of the Command::Definition class

I agree currently it's quite useless. I didn't explain, sorry. I thought it would be good to add additional features to configure the run of a command:

• Add ability to pass stdin. This string would be written to stdin of the command. Useful for example for echo -e "2\n1" | sort
• Add ability to pass environment variables. Useful when the program expects some environment variables passed for configuration.
  How about introducing an options argument so that it's also flexible for the future?:

Command.run("sort", {:stdin => "2\n1\n", :env => {"PATH" => "/usr/local/bin"}})

Apart from that I think it might be useful to have a "container" object which can hold the definition of a command. This object could be serialized, passed along to other parts of a system etc, without having the code to actually run the command attached to it.
What do you think?

However, there's something that Open3 can do that Command cannot.
There's no way (that I can find) to make Command reliably combine the stdout and stderr into one output [...]

That's also what I want to accomplish. Did you want to not use popen3?
I tried a bit, and think have a reasonably well working solution: #4

[njam]

@laserlemon do you think the proposed changes are something you're interested in incorporating into command? Let me know any concerns you have, I'm happy to discuss :)

[njam]

@laserlemon sooo... how do you like the changes and are do you think you wanna integrate them?

[njam]

Moving this to a new repo: https://github.com/cargomedia/komenda