-
Notifications
You must be signed in to change notification settings - Fork 142
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
add format overriding by environment variables #722
add format overriding by environment variables #722
Conversation
Signed-off-by: Marc Bestmann <marc.bestmann@dlr.de>
friendly ping @adityapande-1995 @methylDragon @wjwwood |
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.
Heya, thanks for the contribution! I'm not in principle opposed to this, but could you add a test to the logging test for this? That would help prevent regressions!
Signed-off-by: Marc Bestmann <marc.bestmann@dlr.de>
Sorry for the long wait. This was fairly low-priority for me and I only now had time to work on it again. |
I also added a draft PR for documenting this PR ros2/ros2_documentation#4061 |
friendly ping @methylDragon |
@SammyRamone thanks for the patience ! The team had a discussion about this, and while we do think there should be a holistic way to configure all of this logging, this is a good feature. I'll run full CI on this. |
This is a proposed fix for ros2/launch_ros#368
Originally, I planned on reusing the
RCUTILS_CONSOLE_OUTPUT_FORMAT
environment variable for this. Hoewever, this is not possible since launch and rcutils rely on two different logging implementations which have different names for the same things. For example one supports {line_number} and the other {lineno}. Instead I added two environment variablesOVERRIDE_LAUNCH_SCREEN_FORMAT
andOVERRIDE_LAUNCH_LOG_FORMAT
since screen and log formatting is handled differently in the code.I also framed the variables as override similar to #713
Any feedback is welcome.