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

use yaml for parsing msg and srv values #19

Merged
merged 1 commit into from
Jun 27, 2017
Merged

Conversation

wjwwood
Copy link
Member

@wjwwood wjwwood commented Jun 27, 2017

This should be backwards compatible with the JSON style inputs. Also, these packages already needed yaml, so it's not a new dependency. It also makes using this feature on Windows easier since you don't have to so quote escaping in a lot of cases.

@Kukanani FYI you should be able to do ros2 topic pub /chatter std_msgs/String "data: Hello world" with this pr.

@wjwwood wjwwood added the in review Waiting for review (Kanban column) label Jun 27, 2017
@wjwwood wjwwood self-assigned this Jun 27, 2017
Copy link
Member

@mikaelarguedas mikaelarguedas left a comment

Choose a reason for hiding this comment

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

I like that that will be closer to ros1 and less painful to write!
Tested locally (on linux) 👍

@dhood
Copy link
Member

dhood commented Jun 27, 2017

I am in favour of this because I was annoyed I couldn't copy-paste stuff from ROS 1

@wjwwood wjwwood changed the base branch from various_fixes to master June 27, 2017 16:08
@wjwwood
Copy link
Member Author

wjwwood commented Jun 27, 2017

I changed the base to be master now that #15 is merged.

@wjwwood
Copy link
Member Author

wjwwood commented Jun 27, 2017

We still need to make a decision on whether or not this should get merged today. CI:

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

Copy link
Member

@dirk-thomas dirk-thomas left a comment

Choose a reason for hiding this comment

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

Sounds good to me to merge as soon as CI has passed (though CI won't test for the changed behavior).

@wjwwood
Copy link
Member Author

wjwwood commented Jun 27, 2017

I tested the behavior manually. I'll go ahead an merge since we've got a few pending changes that must go in anyways.

@wjwwood wjwwood merged commit 5e336e3 into master Jun 27, 2017
@wjwwood wjwwood deleted the wjwwood/various_fixes branch June 27, 2017 19:02
@wjwwood wjwwood removed the in review Waiting for review (Kanban column) label Jun 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants