ros2 / launch

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

Addresses issue #588, allowing dict for 'output' #640

Closed m-elwin closed 2 years ago

m-elwin commented 2 years ago

Here is one approach to fixing #588.

I didn't add a test but I'm happy to work on one if someone could point me in the right direction (e.g., where to put the test and what test file I can model it off of).

In issue #577 , the 'output' option for logging in launch files was not participating in $(var) expansion (e.g., in xml launch files).

The fix for this issue (commit 8006dfadc) appears to have broken the use-case where the 'output' is specified as a dictionary (e.g., in python launch files).

This commit only performs the command expansion if the 'output' value is not a dict(), otherwise the dict() is passed through directly.

The 'output' value ultimately is passed to launch/launch/logging/init.py: get_output_loggers() which can accept either a string or a dict to specify the outputs

Signed-off-by: Matthew Elwin elwin@northwestern.edu

mjeronimo commented 2 years ago

@m-elwin How about a test in launch/test/launch? Perhaps test_output.py? Similar to the other tests there that test the basic functionality (test_action, test_condition, etc.) The test_output.py would add testing the output options, including this one where the dict is used for 'output'.

mjeronimo commented 2 years ago
mjeronimo commented 2 years ago
m-elwin commented 2 years ago

Hi @mjeronimo, thanks for merging. Is it possible to get the fix into humble as well?

adityapande-1995 commented 2 years ago

https://github.com/Mergifyio backport humble

mergify[bot] commented 2 years ago

backport humble

✅ Backports have been created

* [#647 Addresses issue #588, allowing dict for 'output' (backport #640)](https://github.com/ros2/launch/pull/647) has been created for branch `humble`