Have cmdrunner use a tty #988
Conversation
modules/cmdrunner/widget.go
Outdated
| err := cmd.Run() | ||
| f, err := pty.Start(cmd) | ||
| if err != nil { | ||
| panic(err) |
There was a problem hiding this comment.
I don't think we should panic here. If the pty fails for some reason, we should fall back to the old way to still provide command output.
There was a problem hiding this comment.
Thanks. Major brainfart on my behalf.
|
This looks great. I'll try to get it in today. |
|
Not sure if it matters, but pty is unlikely to work on windows, which doesn't have them. I'm not sure (I don't have windows :-), maybe the pty module builds and gently errors out, instead of just failing to compile? EDIT - hm, maybe it does work on Windows, see creack/pty#67 Doesn't have to be now, but running in a (pseudo)tty should perhaps become optional. Some commands change their behaviour based on whether they are in a tty or not, for example, they might assume that a progress bar should be written if running in a tty because its a user running it interactively, but that might not be wanted if the command is running in wtf and only the output is wanted. Just a thought. |
|
@sam-github I hadn't thought about that. I added this mostly because if you look at the linked issue people were complaining that commands were not getting colorized be default, and they felt that was unexpected. Perhaps another option is to not even do this and just have additional wording around colorization in the docs. |
|
That's a great point, and it does matter. There are people using this on Windows and we should not break things on them needlessly. Make it opt-in via config? |
|
I like colours, I think this solves a real problem, I didn't mean to suggest it wasn't useful. Executables really do change their behaviour based on The downside is it might not be backwards compat, but I don't think wtf cares too much about that. I don't even think you should make it configurable if you don't have the interest, that can happen if someone actually has a problem, and wants to PR that themselves. |
|
"The downside is it might not be backwards compat, but I don't think wtf cares too much about that" We care very much about backwards-compatibility :) |
|
The things we do for backwards compatability :) |
Seanstoppable
force-pushed
the
cmdrunnertty
branch
2 times, most recently
from
0576d87
to
9a609a6
Compare
This lets us get proper coloring Fixes wtfutil#577
Don't panic Add function due to reuse Catch all errors to appease CI
Seanstoppable
force-pushed
the
cmdrunnertty
branch
from
9a609a6
to
e23cec2
Compare
|
Something strange is going on with this one. It's blocking my local test run and the Travis test run when it gets to the tests in It doesn't look like it needs the |
|
Sounds good. I upgraded to see if it would help with coloring a bit, but didn't seem to have an impact. |
|
Nice! |
This lets us get proper coloring
Fixes #577
Looking at the below, it looks like something is not clearing the color correctly (likely another underlying library bug)
Raw command line:
CmdRunner pre-commit:
CmdRunner pre-commit, forcing color:
CmdRunner post-commit:
