Skip to content
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

Option to strip color escape sequences #639

Open
jacobperron opened this issue Aug 3, 2022 · 3 comments
Open

Option to strip color escape sequences #639

jacobperron opened this issue Aug 3, 2022 · 3 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@jacobperron
Copy link
Member

Feature request

Follow-up to #104 (comment), 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 jacobperron added the enhancement New feature or request label Aug 3, 2022
@jacobperron
Copy link
Member Author

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.

@clalancette clalancette added the help wanted Extra attention is needed label Aug 18, 2022
@bbworld1
Copy link

bbworld1 commented Aug 28, 2022

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
Copy link
Member Author

@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:

  • I would instead use the term "formatter" instead of "filter", for modifying logged records.
    • This would be consistent with Python's logging module (see Formatter vs. Filter)
  • What if we accepted a list of callables for (instead of regex's)?
    • E.g. each callable would take the record to be logged as input and return the modified record.
    • I think this is more flexible and we could provide a strip_regex (or strip_color) function for convenience.
    • Presuming we use the logging.Formatter class, I think this would match the API better.
  • For convenience, it would be nice to add a public variable (or function) containing a regex for stripping color.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants