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.

Sign up for GitHub

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

Jump to bottom

Fix push-ros-namespace in xml/yaml launch files #100

Merged
merged 2 commits into from
Dec 4, 2019
Merged

Fix push-ros-namespace in xml/yaml launch files #100

merged 2 commits into from
Dec 4, 2019

Conversation

ivanpauno
Copy link
Member

expose_action was missing, and there were no test for markup based launch files.

@ivanpauno
Expose push_ros_namespace to frontend based launch files
a4a2912 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno
Corrections after testing
bb5e616 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno ivanpauno added bug Something isn't working in review labels Dec 2, 2019
@ivanpauno ivanpauno requested a review from hidmic December 2, 2019 15:15
@ivanpauno ivanpauno self-assigned this Dec 2, 2019
@ivanpauno
Copy link
Member Author

Should the tag be push_ros_namespace or push-ros-namespace?
The first one is used in the tutorial https://github.com/ros2/ros2_documentation/blob/master/source/Tutorials/Launch-files-migration-guide.rst. The second option (which I used in this PR), is being used by substitutions (like find-pkg-share). We don't have currently a multi-worded action name in launch/launch_ros, exposed to xml/yaml based launch files. I prefer the underscore style (and I don't mind to keep the dash for substitutions).

hidmic
hidmic approved these changes Dec 2, 2019
View reviewed changes
Copy link
Contributor

@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 pending green CI

@hidmic
Copy link
Contributor

hidmic commented Dec 2, 2019 •
edited
Loading

Should the tag be push_ros_namespace or push-ros-namespace?

Well, dashes are much more common in XML, though underscores look nicer in YAML. IMHO this is a valid argument to support tag aliasing. For now, though, I'd personally and subjectively go with dashes.

wjwwood
wjwwood approved these changes Dec 2, 2019
View reviewed changes
Copy link
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 too, with CI.

I agree that dashes look better in XML. I would support allowing both or allowing the markup to dictate one versus the other.

@ivanpauno
Copy link
Member Author

ivanpauno commented Dec 2, 2019 •
edited
Loading

  • Linux Build Status
  • Arch Build Status
  • macOS Build Status
  • Windows Build Status

@ivanpauno ivanpauno merged commit 1d88418 into master Dec 4, 2019
@delete-merged-branch delete-merged-branch bot deleted the ivanpauno/correct-push-ros-namespace-frontend branch December 4, 2019 18:39
@mjcarroll
Copy link
Member

@ivanpauno Can you get this backported please?

ivanpauno added a commit that referenced this pull request Jan 17, 2020
@ivanpauno
Fix push-ros-namespace in xml/yaml launch files (#100)
f08a68b 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno ivanpauno mentioned this pull request Jan 17, 2020
Merged
@ivanpauno
Copy link
Member Author

@ivanpauno Can you get this backported please?

See #115.

ivanpauno added a commit that referenced this pull request Jan 17, 2020
@ivanpauno
Fix push-ros-namespace in xml/yaml launch files (#100)
5202560 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: ivan <ivanpauno@gmail.com>
ivanpauno added a commit that referenced this pull request Jan 21, 2020
@ivanpauno @BMarchi
@mxgrey @nuclearsandwich @wjwwood @jacobperron
Eloquent patch release 1 (#115)
f586dbb 
* Add pid to launch_ros node name as suffix

Signed-off-by: Brian Ezequiel Marchi <brian.marchi65@gmail.com>
Signed-off-by: ivan <ivanpauno@gmail.com>

* Pass the node-name attribute through the substitution parser (#101)

Signed-off-by: ivan <ivanpauno@gmail.com>

* Fix push-ros-namespace in xml/yaml launch files (#100)

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: ivan <ivanpauno@gmail.com>

* Maintain order of parameters regarding name and from (#99)

Signed-off-by: Brian Ezequiel Marchi <brian.marchi65@gmail.com>
Signed-off-by: ivan <ivanpauno@gmail.com>

* Use imperative mood in constructor docstrings. (#103)

* Use imperative mood in constructor docstrings.

Fixes D401 in pycodestyle 5.0.0 and flake8.

Signed-off-by: Steven! Ragnarök <steven@nuclearsandwich.com>

* Use imperative mood in docstring.

Signed-off-by: Steven! Ragnarök <steven@nuclearsandwich.com>

* Use imperative mood in docstrings.

Signed-off-by: Steven! Ragnarök <steven@nuclearsandwich.com>
Signed-off-by: ivan <ivanpauno@gmail.com>

* Fix misleading deprecated warnings when using launch arguments (#106)

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: ivan <ivanpauno@gmail.com>

* check for shutdown while waiting for a service response to avoid hang during shutdown (#104)

* check for shutdown while waiting for a service response to avoid hang during shutdown

Signed-off-by: William Woodall <william@osrfoundation.org>

* fix typo in logger call

Signed-off-by: William Woodall <william@osrfoundation.org>
Signed-off-by: ivan <ivanpauno@gmail.com>

* Fix frontend topic remapping (#111)

* Add frontend remap test

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Pass data_type parameter to remap entity

This resolves an issue where frontend remaps are not parsed.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: ivan <ivanpauno@gmail.com>

Co-authored-by: Brian Marchi <brian.marchi65@gmail.com>
Co-authored-by: Grey <grey@openrobotics.org>
Co-authored-by: Steven! Ragnarök <nuclearsandwich@users.noreply.github.com>
Co-authored-by: William Woodall <william+github@osrfoundation.org>
Co-authored-by: Jacob Perron <jacob@openrobotics.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hidmic hidmic hidmic approved these changes

@wjwwood wjwwood wjwwood approved these changes

Assignees

@ivanpauno ivanpauno

Labels
bug Something isn't working in review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ivanpauno @hidmic @mjcarroll @wjwwood