ros2 / launch

Tools for launching multiple processes and for writing tests involving multiple processes.
Apache License 2.0
126 stars 141 forks source link

Option to strip color escape sequences #639

Open jacobperron opened 2 years ago

jacobperron commented 2 years ago

Feature request

Follow-up to https://github.com/ros2/launch/issues/104#issuecomment-424860566, it would be nice if launch supported stripping color escape sequences when logging output to a file.

Feature description

For example, the following launch file will print Hello colorful world with red text on a green background:

import launch
from launch.actions import ExecuteProcess

def generate_launch_description():
    return launch.LaunchDescription([
        ExecuteProcess(cmd=['echo', '-e', '\e[7;32;41mHello colorful world\e[0m'], output='both')
    ])

You should see the colorful text on screen, and in the log file there will be color codes surrounding the text.

For some applications it would be nice to strip out any color escape sequences from the log file. I think it would be nice if ExecuteProcess provided an option for this, e.g. strip_color=True.

Implementation considerations

We should probably keep the current behavior, keeping color escape sequences in the log file, as the default. This avoids breaking anyone relying on them when updating.

jacobperron commented 2 years ago

We could offer a single option to strip out color escape sequences for all logs (to file and screen), or offer separate options to configure the file and screen separately. I guess the latter would be preferred if the implementation is not too difficult.

bbworld1 commented 2 years ago

If this issue is open, could I work on it? New to contributing here.

We could offer a single option to strip out color escape sequences for all logs (to file and screen), or offer separate options to configure the file and screen separately.

Re: this, I had the more general idea of allowing specifying general regex filters, which would allow filtering out not only color escape sequences, but any type of data that you might not want to display in logs. My current idea is to tack on a "filter_out" option onto the get_logger function in launch/logging/__init__.py and then have higher-level constructs up to ExecuteProcess pass this parameter down when the loggers are initialized, but if you have a better idea I'd definitely be open to that. It would look like this in the example above:

import launch
from launch.actions import ExecuteProcess

def generate_launch_description():
    return launch.LaunchDescription([
        ExecuteProcess(cmd=['echo', '-e', '\e[7;32;41mHello colorful world\e[0m'], output='both', filter_out=["\e\[[0-9;]*m"])
    ])

or for individual configuration:

def generate_launch_description():
    return launch.LaunchDescription([
        ExecuteProcess(cmd=['echo', '-e', '\e[7;32;41mHello colorful world\e[0m'], output='both', filter_out={'log': ["\e\[[0-9;]*m"]})
    ])

where we'd detect whether to handle individually based on whether a list or dict is passed.

jacobperron commented 2 years ago

@bbworld1 I don't believe anyone is looking into this issue at the moment, so your contribution is welcome!

Your proposal makes sense to me. Some other ideas building on top of it: