-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Rewrite of old rosidl tutorial #614
Conversation
Signed-off-by: maryaB-osr <[email protected]>
Signed-off-by: maryaB-osr <[email protected]>
Signed-off-by: maryaB-osr <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM but for a few details.
Signed-off-by: maryaB-osr <[email protected]>
Signed-off-by: maryaB-osr <[email protected]>
Signed-off-by: maryaB-osr <[email protected]>
Signed-off-by: maryaB-osr <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, I like what you've done here. I think this is a good article to have.
One thing that I think we should change is the name of the article. "More-Interfaces" isn't very descriptive. I'd go with something like "Single-Package-Define-and-Use-Interface", but anything along those lines is OK with me. I've got a few other small things inline, though I didn't do a full review this time. Once things are renamed I can do another look over.
Signed-off-by: maryaB-osr <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, I like the direction this one is going in. I've done a more thorough review now and have left a few more comments for you to think about.
.. note:: | ||
|
||
You can use an existing interface definition in a new interface definition. | ||
For example, the `rosidl_tutorials_msgs package <https://github.com/ros2/tutorials/blob/rosidl_tutorials/rosidl_tutorials/rosidl_tutorials_msgs/msg/Contact.msg>`_ on GitHub has a msg named ``Contact.msg``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, didn't we decide not to point to the tutorials repository as it doesn't really work right now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it looks bad :/ but that's the msg we're using for the following example to show that you can use existing interfaces to define new ones.
Aside from having them create Contact.msg
themselves in another package (meaning they'll have to go create another package just for that), or rewriting the example to use an existing msg from some up-to-date package in the ecosystem (which would also make the tutorial longer because a completely new publisher would have to be written to accommodate a different msg), I don't see a way around linking to Contact
.
I rewrote the sentence to take the focus off the rosidl_tutorials
package at least:
For example, let's use
Contact.msg <https://github.com/ros2/tutorials/blob/rosidl_tutorials/rosidl_tutorials/rosidl_tutorials_msgs/msg/Contact.msg>
_, which belongs to an existing ROS 2 package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough. I don't love it, but since that is the point of that section, so be it.
Signed-off-by: maryaB-osr <[email protected]>
The old rosidl tutorial had some concepts in it that I didn't cover in my new "simple" interface tutorial (because it's already too long) so instead of deleting the old one, I rewrote it and removed all the repeated information between the two.
The code works now but I'm not sure if it's "right".
This bit of code in the original:
works, but I replaced it with this because it's much shorter:
Is there a different situation for each of these? If that longer block is meant for a specific purpose, I'd like to include that in the tutorial.
fixes #565