Colorized output for supervisorctl status #530
Conversation
| @@ -22,7 +22,8 @@ | |||
| elif (3, 0) < py_version < (3, 2): | |||
| raise RuntimeError('On Python 3, Supervisor requires Python 3.2 or later') | |||
|
|
|||
| requires = ['meld3 >= 1.0.0'] | |||
| requires = ['meld3 >= 1.0.0', | |||
| 'colorama'] | |||
There was a problem hiding this comment.
I hope that we do not add an external dependency just to get some colored output. I think a feature of Supervisor is that it has no dependencies besides meld3 (which is our package too), and at some point I'd like to find a way to remove meld3. Supervisor is widely used by non-Python people and is packaged by a lot of distributions. Having fewer dependencies makes it easier to install and package for those people.
There was a problem hiding this comment.
If that's the only concern, I can pull in some color constants.
colorama does something fancier to make colors work in Windows, but we could make that optional.
There was a problem hiding this comment.
I could even change this so that it only does the import colorama when the user turns on coloring (which is not on by default). If they turn on coloring and the packages are not installed, then I can say "Colorized output requires the 'colorama' package". Could even combine this idea with the one above so that color codes are vendored in and colorizing works out of the box on platforms other than windows. The message about colorama would only be installed if on Windows and the user requests color.
There was a problem hiding this comment.
I can pull in some color constants.
I think it would probably be fine to just emit some escape sequences, pretty much all modern terminals support vt10x codes, and then everyone could have color without the dependency. I don't know if supervisorctl works on Windows (we don't officially support Windows at all). If it does then there's only a very small number of users running it that way, and not having color would probably not be a big deal. They could get color with cygwin.
The color feature could be done in a ctlplugin but I think enough people would like this where it's probably worth considering putting it into supervisorctl. There is a lot that could be done to improve supervisorctl. We did some work on fixing command completion some months ago (I think you had a patch in that) but I'd really like for error messages to go to stderr and every command to have a defined set of exitcodes. It just hasn't been a priority because of test coverage, Python 3, etc.
|
OK, I switched from |
msabramo
force-pushed
the
colorized_output
branch
from
cc11ab4
to
4b4a85b
Compare
|
+1 |
|
I'll try to come back to this in a few days. Any changes/ideas? |
|
The colorized output looks like a great addition 馃憤 |
Enable it by adding to config file:
[supervisorctl]
colorize_status = auto
msabramo
force-pushed
the
colorized_output
branch
from
4b4a85b
to
880bbfb
Compare
|
Squashed it down to a single commit. |
|
+1 |
|
Thanks for updating this patch to remove the dependency. I think this is a nice addition and I would be happy to merge this if some test coverage was added. I looked through the new patch and there's one thing I would like to discuss: Right now the option to use color is specific to the status command. Should we add one "use color" option that affects all of supervisorctl, or is there a use case for specifying color/no color per command? There might be other uses for color, e.g. we might want to show errors in red, or plugins might want to colorize something. I'm wondering if we will accumulate color options for those cases or if one global "use color" option would be sufficient. |
|
Interesting. I had only thought of using color for |
|
Why was this PR never considered to be merged ? I find the feature mega useful. |
|
when the target to output is not a terminal, such as pipe, I think it is better not to output colorfully. |
How about a sexy new feature for 4.0...? 馃槃
I've long wanted this, because it makes the failed processes stand out from the OK ones in a long list.
I'm toying with the idea of colorizing the entire line rather than just the status. Or maybe the name and the status. Having the name colorized seems nice so that your eyes can just scan the first column. Kind of thinking of not colorizing the rest of the line according to status, because I had an idea of maybe colorizing the uptime if it less than a certain threshold (
startsecs?) -- this is because we sometimes have processes that repeatedly spin up and die and use a lot of CPU, so it can be very useful sometimes to spot the processes that have the shortest uptime...@mcdonc and @mnaberez: What do you guys think?
I didn't write tests, but I will add them if you're cool with the feature.