Skip to content

Support xml and yaml files in ros2launch#40

Merged
ivanpauno merged 6 commits intomasterfrom
ivanpauno/ros2launch-support-xml-yaml-files
Aug 1, 2019
Merged

Support xml and yaml files in ros2launch#40
ivanpauno merged 6 commits intomasterfrom
ivanpauno/ros2launch-support-xml-yaml-files

Conversation

@ivanpauno
Copy link
Copy Markdown
Member

@ivanpauno ivanpauno commented Jul 12, 2019

What the title says.
The first commit doesn't depend on any PR. After the second commit, it depends on ros2/launch#271, which allows better error handling a cleaner code.

TODO: Add tests. I've tested locally, and it works fine.

ivanpauno added 2 commits July 12, 2019 11:37
@ivanpauno
Add support to XML and YAML files in ros2launch
63daf07 
Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
@ivanpauno
Updated after refactor in launch
1ba59a7 
Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
@ivanpauno ivanpauno added enhancement New feature or request in progress labels Jul 12, 2019
@ivanpauno ivanpauno requested a review from hidmic July 12, 2019 17:42
@ivanpauno ivanpauno self-assigned this Jul 12, 2019
@ivanpauno ivanpauno changed the title Support xml yaml files Support xml and yaml files in ros2launch Jul 12, 2019
hidmic
hidmic approved these changes Jul 19, 2019
View reviewed changes
Copy link
Copy Markdown

@hidmic hidmic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, though is it .launch.py or _launch.py? IIRC there were some discussions, and people were advocating for the latter (as a python script suffixed with .launch.py cannot be imported). @wjwwood ?

@sloretz
Copy link
Copy Markdown
Contributor

sloretz commented Jul 19, 2019

@hidmic ros2 launch tab completion works with both, though I was under the impression _launch.py was preferred too.

wjwwood
wjwwood reviewed Jul 19, 2019
View reviewed changes
@wjwwood
Copy link
Copy Markdown
Member

wjwwood commented Jul 19, 2019

Personally I still like .launch.py as I don't foresee my self importing it often and/or I don't mind importing it with importlib. For xml and yaml there's no reason to _ instead unless we just want to use that everywhere. I don't remember where we left the conversion on what we recommend, but we didn't change any of the existing examples, e.g.:

https://github.com/ros2/demos/blob/dashing/demo_nodes_cpp/launch/topics/talker_listener.launch.py

ivanpauno added 3 commits July 22, 2019 11:37
@ivanpauno
Address review feedback
b7c14fb 
Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
@ivanpauno
Address review feedback
dee85f0 
Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
@ivanpauno
Correct error after testing
bd1a315 
Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
@ivanpauno ivanpauno mentioned this pull request Jul 26, 2019
Merged
@ivanpauno ivanpauno requested review from hidmic and wjwwood July 30, 2019 13:52
wjwwood
wjwwood reviewed Jul 30, 2019
View reviewed changes
wjwwood
wjwwood approved these changes Jul 30, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, I'll re-review if you decide to refactor the error handling again.

@ivanpauno
Copy link
Copy Markdown
Member Author

ivanpauno commented Jul 31, 2019

CI (up to ros2launch launch launch_xml launch_yaml launch_ros test_launch_ros, only fastrtps):

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@ivanpauno
Please flake8
e815b4b 
Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
@ivanpauno
Copy link
Copy Markdown
Member Author

ivanpauno commented Aug 1, 2019

  • Linux Build Status

@ivanpauno ivanpauno merged commit bb8fd1a into master Aug 1, 2019
@delete-merged-branch delete-merged-branch bot deleted the ivanpauno/ros2launch-support-xml-yaml-files branch August 1, 2019 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wjwwood wjwwood wjwwood approved these changes

@hidmic hidmic Awaiting requested review from hidmic

Assignees

@ivanpauno ivanpauno

Labels

enhancement New feature or request in progress

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ivanpauno @sloretz @wjwwood @hidmic