phase2 / rig

Outrigger command line tool
MIT License
11 stars 8 forks source link

Display executed commands via Verbose Logging #111

Closed grayside closed 6 years ago

grayside commented 7 years ago

Finally getting back to the learnification.

This is still in-progress but wanted to get an approach confirmation before continuing.

3__thanks_for_flying_vim__zsh_

Fixes #5

febbraro commented 7 years ago

Maybe we should use util.Run for everything instead of exec.Command and then we can just create the command to run? like maybe util.Run returns an exec.Cmd so you can still choose to Run or CombineOutput or whatever? I realize we dont always handle Cmds in a consistent way, but seems more clear/simple than wrapping every exec.Command call

grayside commented 7 years ago

The problem there is if we output the log when we create the command, that could be out of sequence when we actually execute it. Certainly wrapping the exec.Command factory is part of the strategy in #97 so I don't object on that point.

Would it make sense to extend exec.Cmd into our own struct and shadow Run et al with the verbose logging?

febbraro commented 7 years ago

Shadowing might be good, sometimes we don't call Run() directly though, but call CombinedOutput() or Output(), I wonder if shadowing Run() would get our method called, or the base struct?

grayside commented 7 years ago

To do this at exec time, I assume we need to shadow each unless the API internals use a common exec call we can shadow.

grayside commented 6 years ago

Moved command output to yellow for better visibility. Shifted to more of an object wrapper model. Stopped implementation for approach review.

febbraro commented 6 years ago

I like how this is looking a lot better. The code that needs to run commands look really no different and is still pretty easy to read, and the Executor looks pretty simple.

grayside commented 6 years ago

Too much?

3__root_bbefb61ec982__var_www_northwell-d8__zsh_

grayside commented 6 years ago

Note that in the example above, we see that our use of machine.GetData() could maybe do some static caching. Likely a nice little performance optimization.