-
Notifications
You must be signed in to change notification settings - Fork 93
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CLI: a splash of colour #3096
CLI: a splash of colour #3096
Conversation
Some general comments first: I definitely like colour output for the CLI Colorama docs say: "ANSI escape character sequences have long been used to produce colored terminal text and cursor positioning on Unix and Macs. Colorama makes this work on Windows, too, by wrapping stdout, stripping ANSI sequences it finds (which would appear as gobbledygook in the output), and converting them into the appropriate win32 calls to modify the state of the terminal. On other platforms, Colorama does nothing." i.e. "[on Linux] Colorama does nothing", and its main purpose is to translate ANSI color control codes to the MS Windows API (which we don't support). Presumably in spite of the "it does nothing [on Linux]" statement, it is nicer to use than raw ANSI control codes.
Should we use ansimarkup then?? |
7a54af4
to
a6c0649
Compare
No objections so I've moved this along:
Colour support so far:
If no-one screams I'll complete the conversion, rename @kinow I'm guessing you're not going to like my wrapping of the CLI methods as the wrapper populates the arguments. Happy to change, open to suggestions, trying not to write any solution into |
a6c0649
to
10e6a84
Compare
10e6a84
to
43b5f5d
Compare
43b5f5d
to
f5fa629
Compare
Tested with a running suite, then validate and show, and show got some colours (validate was green, same colour as my terminal background, so maybe there was some colouring, but I couldn't notice).
Does it add/help a lot? Last commit in 2018, and few contributors - which could be fine, as the tool is simple, it could be feature-complete and only fix bugs (which it doesn't have any at the moment in github). But if it does add or simplify the code, and you think it's stable to be used in Cylc, all good 👍
No suggestion. I have never used anything like this before, so it looks quite alien for me, but not incorrect. I think it is OK to be added now and we can think in alternatives later if we find issues with the current approach, or if there are simpler alternatives. (not reviewing as it's marked as discussion, so I think others will want to have a look and give their opinion too before we jump to reviewing/merging?) |
You have a green terminal background! Not too sure what can be done about this one, it might be possible to detect the terminal background ANSI code, then pair the corresponding foreground code with a white background. What do you see if you see with tools like
It simplifies things somewhat making code more readable (cleaner syntax, no string formatting required), it also adds 8-bit colour support (where the terminal supports it) as used to turn the DEBUG messages grey (there is no grey ANSI code). I think
I'm not overjoyed by it as it causes linters to complain, but it's a perfectly valid Python pattern. Happy to change later for a nicer approach if we come up with one. |
Oh sorry, actually green text color! Hahaa, would be pretty hard on my eyes a green background haha |
|
f5fa629
to
45e6228
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Sanity check code
- Try it out on desktop
- With
--color=[always|auto|never]
- With my theme and a corner case "nasty" theme that I keep hanging around for this sort of thing. It looks no worse than the theme does anyway.
- Used a color blindness simulator on the output. It's not as much of an improvement on the uncolored version, but it's not a degradation. :)
- With
Otherwise LGTM (not that the codacy problem is your fault; Pylint is just deficient). |
45e6228
to
874543f
Compare
Spurious test battery failure on Travis CI. |
🌈 |
Add colour support to the
cli_function
wrapper.auto
which will use colour if displayed in a terminal.ansimarkup
library for easy colour control:<red>Error</red>
.cylc cat-log
sys.exit
toraise CylcError
Note: The
cli_function
wrapper may change with #2972, colour support is somewhat orthogonal to this.Example