-
Notifications
You must be signed in to change notification settings - Fork 265
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
Add some introductory material to the README, and a Tips and Tricks section #1916
Add some introductory material to the README, and a Tips and Tricks section #1916
Conversation
8e2b2ae
to
1963f70
Compare
…ection Signed-off-by: Emerson Knapp <[email protected]>
1963f70
to
2fb8a48
Compare
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.
Just a minor comment
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.
Simple changes
README.md
Outdated
@@ -2,7 +2,18 @@ | |||
 | |||
[](https://github.com/ros2/rosbag2/actions) | |||
|
|||
Repository for implementing rosbag2 as described in its corresponding [design article](https://github.com/ros2/design/blob/ros2bags/articles/rosbags.md). | |||
Rosbag2 - the tool for recording and playback of messages from ROS 2 topics. |
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.
I would use "rosbag2" here even at the start of a sentence, since this is how it's written in other places.
Rosbag2 - the tool for recording and playback of messages from ROS 2 topics. | |
rosbag2 -- the tool for recording and playback of messages from ROS 2 topics. |
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.
and now it actually supports services
too.
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.
I actually prefer to use Rosbag2 starting from the capital R everywhere in the documentation and presentations.
The "rosbag2" is a package name, and it should be used with the small starting r when we mention this specific package name.
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.
That makes sense to me, but the README doesn't seem to adhere to that, or at least it's not very consistent. Anyway, we can leave it as-is and improve that later.
I would however do this:
Rosbag2 - the tool for recording and playback of messages from ROS 2 topics. | |
Rosbag2 -- the tool for recording and playback of messages from ROS 2 topics. |
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.
Updated all the usage to Rosbag2 because why not.
Also, switched to an actual — em dash character instead of multi-dash, which doesn't render that way in all situations.
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.
lgtm after all comments are resolved.
README.md
Outdated
@@ -2,7 +2,18 @@ | |||
 | |||
[](https://github.com/ros2/rosbag2/actions) | |||
|
|||
Repository for implementing rosbag2 as described in its corresponding [design article](https://github.com/ros2/design/blob/ros2bags/articles/rosbags.md). | |||
Rosbag2 - the tool for recording and playback of messages from ROS 2 topics. |
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.
and now it actually supports services
too.
Co-authored-by: Alejandro Hernández Cordero <[email protected]> Signed-off-by: Emerson Knapp <[email protected]>
Co-authored-by: Christophe Bedard <[email protected]> Signed-off-by: Emerson Knapp <[email protected]>
Co-authored-by: Christophe Bedard <[email protected]> Signed-off-by: Emerson Knapp <[email protected]>
Co-authored-by: Christophe Bedard <[email protected]> Signed-off-by: Emerson Knapp <[email protected]>
Signed-off-by: Emerson Knapp <[email protected]>
Signed-off-by: Emerson Knapp <[email protected]>
Signed-off-by: Emerson Knapp <[email protected]>
Believe I resolved all the comments with the latest - including @fujitatomoya I changed the intro language to
|
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.
im good to go. i will leave this @christophebedard and @MichaelOrlov
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.
Looks good!
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.
LGTM.
The comment "this is already possible, but it's maybe not obvious" came up in two recent issue conversations, so I've added a new section "Tips & Tricks" to the
README.md
and added some clarifying introductory material as well to help navigate to it.Conversations of note at #1910 (comment) and #1595 (comment)