Revisit color support for poutput, perror, etc.

[tleonhardt]

Currently the signature for poutput and perror are as follows:

def poutput(self, msg: Any, end: str = '\n', color: str = '') -> None:
    pass

def perror(self, err: Union[str, Exception], traceback_war: bool = True, err_color: str = Fore.LIGHTRED_EX,
               war_color: str = Fore.LIGHTYELLOW_EX) -> None:
    pass

Where color, err_color and war_color are optional ANSI escape codes to style/format the output with and if present, we reset the foreground color after the output. However, we never considered that someone may set the background color or other effects and we do not reset those.

We should probably refactor these methods to take separate foreground and background styles such as:

def poutput(self, msg: Any, end: str = '\n', fg: str = '', bg: str = '') -> None:
    pass

And the foreground and background should be reset respectively at the end depending on what is provided.

We may also want to consider making these more user friendly where the user can pass in an English name for a color such as 'green' and we have two dictionaries maintained inside the cmd2.Cmd instance called something like self.fg_color and self.bg_color which map a string to an ANSI escape sequence. These dictionaries could be pre-populated with default styles for common colors as well as logical situations like warning, error, success, etc. But cmd2 application developers would be able to override these default styles for any given color.

[xNinjaKittyx]

I just quickly hacked up something:

    def poutput(self, msg: Any, end: str = '\n', fg: str = '', bg: str = '') -> None:
        if msg is not None and msg != '':
            try:
                values = []
                try:
                    if fg:
                        values.append(getattr(Fore, fg.upper()))
                    if bg:
                        values.append(getattr(Back, bg.upper()))
                except AttributeError:
                     raise ValueError('Color does not exist.')
                values.append(msg)
                values.append(Style.RESET_ALL)

                if end:
                    values.append(end)

                self.decolorized_write(self.stdout, ''.join(values))
            except BrokenPipeError:
                ...

Test code:

import cmd2


class MyApp(cmd2.Cmd):
    def do_foo(self, args):
        self.poutput('something', fg='blue', bg='red')


MyApp().cmdloop()

Just one of many ways possible... getattr feels kind of "hacky". However, having 2 dictionaries that track Foreground + Background seems really awkward...

Problem with having raw ANSI escape codes is that someone could just simply inject ANSI sequences unrelated to fg or bg which doesn't make any sense. Checking for it would be overkill.

self.poutput('something', fg=style.DIM, bg=Fore.RED) would be valid code, unless we added checking..

I also changed the str concatenation to joining a list, since it should be ever so slightly faster.

Click actually has something already pretty similar (they don't use colorama, and just put ansii codes in their code base)
https://github.com/pallets/click/blob/a936b99212c69a36a2cc2828252b2c4280c0c88c/click/termui.py#L372

[tleonhardt]

@xNinjaKittyx Thanks for the suggestion.

I think we could make something like that work. I don't like having to replicate colors that are well defined in colorama, but I also don't want to have to let users enter raw ANI escape codes. At the same time we may want to allow a few "custom" color names for things like error, warn, success, etc. Perhaps we could search a custom dict first and then if an entry is not present we could use the getattr approach you outlined to see if the color exists in colorama.

[xNinjaKittyx]

I want to try doing this:
master...xNinjaKittyx:feature/color-support

I noticed a snippet of code, which I was wondering what the history was behind it in def poutput(...):

                if not msg_str.endswith(end):
                    msg_str += end

Also another difference that seems to trigger some problems is that the original code does a Fore.RESET after the newline... However, doing a Back.RESET after the newline causes the background to bleed into the next line. One of the tests break if this behavior is changed.

I think the correct behavior would be to do ALL resets before the end character.

I really wanted to make the styling a modular function called by both perror & poutput, but it looks like one of the tests break because of the msg = str(msg) conversion. I also wanted the newlines in perror to be part of end="\n" but this also breaks 1 test (which shouldn't be a big deal to change? It's technically broken because a 2nd \n doesn't appear due to snippet of code above if applied to perror)

[tleonhardt]

I really like the idea of having a common style utility function which is leveraged in all cases. If we need to do a little refactoring of code and/or unit tests to make them happen, I'm all for it. The utils.py file is probably the correct location for a function like that.

For the dictionaries, I'd put them in constants.py instead of cmd2.py; but they look good.

I don't recall why we have code to add a newline by default. Maybe it was to preserve legacy behavior? Maybe it was for convenience because we thought users would want it in most cases? Regardless, we should definitely do a RESET (of whatever type we are doing) BEFORE adding the newline. If we didn't before, shame on us ;-)

[xNinjaKittyx]

I think adding a newline by default is ok, since that's very similar to how print() works. It's just a little confusing as to why we don't append the end character if the msg ends with the same character as end.

This seems really odd because someone might want to add a non styled

self.poutput('hello world\n')

and it would be equivalent to

self.poutput('hello world')

Of course, if someone tries to the first one w/ a bg value, it'll be obvious that it wasn't the right way to do it since the background will bleed into the next line.

[tleonhardt]

@kmvanbrunt @kotfu Do either of you have any strong opinions in relation to this?

[kotfu]

Not strong opinions, but a couple of thoughts. I think we should keep the new line character so it works the same way as the python print() function. I also think that the function that applies styling should be separate from the function that prints the output. I'd much prefer something like:

self.perror(style(bg=RED, fg=BLACK, msg='big oopsie'))

instead of:

self.perror('big oopsie', bg=RED, bg=BLACK)

I also think we should continue to use colorama, and should just use their constants, i.e. colorama.Fore.RED. If someone wants to use another color library, say colored, then they would just use:

self.perror(colored.stylize("This is green.", colored.fg("green")))

If a developer using cmd2 wants to always have error stuff use the same color, then they could override perror() and do their own styling on the message argument before calling super.perror().

[kmvanbrunt]

Here are my thoughts:

Since we are already introducing breaking changes, then it might be a good idea to have 3 functions to make the API more intuitive.

1. poutput() - print a string to self.stdout
2. perror() - print a string to sys.stderr
3. pexcept() - print an Exception to sys.stderr

I typically use perror to print strings and remembering to set traceback_war to False is annoying. Since this parameter only applies to exceptions, breaking the current perror into two functions simplifies the API.

As far as the end argument to poutput, I think we need to make the functions consistent. If poutput is going to have the argument, then all 3 functions should have it. Otherwise remove it and have them all work like stdout.write and stderr.write.

If we do keep end as an argument , then eliminate the if from the following block of code to make it work like print().

if not msg_str.endswith(end):
    msg_str += end

[tleonhardt]

@xNinjaKittyx Daniel, if you want to take into consideration the feedback from @kotfu and @kmvanbrunt and update what you've been working on, a PR would be most welcome.

[xNinjaKittyx]

I'll give another update later, but as for the feedback.

I think the 3 method split is a good idea, and to normalize the methods so they have similar arguments (i.e. end)

As for the style, i still think the original idea of having fg/bg is convenient. The user is still allowed to use their own stylizer if they wish to do that themselves.

[xNinjaKittyx]

Hello,

I have my latest changes here: https://github.com/python-cmd2/cmd2/compare/master...xNinjaKittyx:feature/revisit-color-support?expand=1

I'm running into 2 last test errors that I'm not sure where to find the problem.

1. history/multiline history is failing to add a newline after the alias?
2. In notty/tty tests, I'm running into an exception EXCEPTION of type 'TypeError' occurred with message: 'sequence item 3: expected str instance, bool found, but I don't know where this error is coming from.

Edit: I found the problem for 2), but now I have another problem when it comes to terminals that don't support colors. Since Style.RESET_ALL is applied all the time, it returns an AssertionError because it appends the escape sequence when no color is added.