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

Various fixes #142

Merged
merged 14 commits into from Jun 28, 2017
Merged

Various fixes #142

merged 14 commits into from Jun 28, 2017

Conversation

mikaelarguedas
Copy link
Member

@mikaelarguedas mikaelarguedas commented Jun 27, 2017

Not in this PR:

  • move the pendulum demo executables (because they need to be on the path given the current launch logic)

Depends on ros2/robot_state_publisher#8
Depends on ros2/ros2cli#23

@mikaelarguedas mikaelarguedas added the in progress Actively being worked on (Kanban column) label Jun 27, 2017
@mikaelarguedas
Copy link
Member Author

if we dont want to address "make topic_monitor launchfiles runnable with launch" this is ready for review

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, but I don't know how to answer the question about topic_monitor needing to be launch-able by launch.

@dhood
Copy link
Member

dhood commented Jun 27, 2017

sorry, didn't see the 'question' because updating the description doesn't send notifications (nor does editing in an @-mention). This looks good as is and I'll have a look at the topic monitor launching

@mikaelarguedas
Copy link
Member Author

sounds good, thank you both for the the input

@mikaelarguedas
Copy link
Member Author

waiting for ros2/ros2cli#23 to be resolved to update this before merging

@mikaelarguedas
Copy link
Member Author

ok I think this is ready for another review (sorry I rebased so it's not easy to see identify new commits since the last review). I'll wait for ros2/ros2cli#23 to be merged and then merge this along updating all the tutorials accordingly

@dhood I updated the topic_monitor README to match the current state, I'll leave it to you to update anything necessary if you decide to iterate on the launchfiles 😉

@dirk-thomas
Copy link
Member

This might need to be updated when ament/ament_python#2 is merged to use the same approach as in the referenced PRs.

@mikaelarguedas mikaelarguedas merged commit a4d48aa into master Jun 28, 2017
@mikaelarguedas mikaelarguedas deleted the various_fixes branch June 28, 2017 03:04
@mikaelarguedas mikaelarguedas removed the in progress Actively being worked on (Kanban column) label Jun 28, 2017
@mikaelarguedas
Copy link
Member Author

thanks @dhood for the review and fixes!

@dirk-thomas
Copy link
Member

This previous comment still needs to be addressed. Otherwise the moved executables aren't runnable with ros2 run.

@mikaelarguedas
Copy link
Member Author

Yeah I fell asleep before opening a PR. I'll do that this morning

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants