add examples_ prefix to package names to avoid future collisions #137

codebot · 2016-11-22T22:51:00Z

Some of the package names we're coming up with in the examples repo are pretty generic. Prefixing them with examples_ will reduce the likelihood of colliding with other package names.

mikaelarguedas · 2016-11-22T22:53:30Z

should the generated targets be prefixed as well ?

codebot · 2016-11-22T22:55:06Z

That's a good question. Probably. It will make typing them a bit longer, but I suppose talker_timer_lambda and talker_timer_member_function are pretty generic names to be throwing into bin and they may collide with the rclpy examples. I'll play around with a few options.

codebot · 2016-11-22T22:56:22Z

also, what do you think of renaming talker to minimal_publisher ? The names talker and listener are sort of in our vocabulary because we've been using them forever, but they aren't super descriptive.

mikaelarguedas · 2016-11-22T23:03:41Z

yeah I agree, we use publisher/subscriber and requester/replier in a lot of places maybe it would make sense to update the vocabulary everywhere.

codebot · 2016-11-22T23:52:59Z

OK I'll change the package and executable names. Brace for impact.

codebot · 2016-11-23T00:56:21Z

OK it looks somewhat decent now. The build products are all prefixed by examples_rclcpp_ which is kind of nice.

mikaelarguedas · 2016-11-29T00:41:31Z

rclcpp/minimal_subscriber/lambda.cpp

  {
-    subscription_ = this->create_subscription<std_msgs::msg::String>("chatter",
+    subscription_ = this->create_subscription<std_msgs::msg::String>("topic",


nicktip: I think we encourage to wrap after the parenthesis
same below

mikaelarguedas

Changes look good to me.
Is it on purpose that you create structs that inherit from rclcpp::Node rather than classes ?

codebot · 2016-11-29T00:54:51Z

I did struct instead of class just because that's what was going on in the previous examples. I think it just lets you avoid wasting a line for public:

dirk-thomas · 2016-11-29T15:21:23Z

rclcpp/minimal_publisher/CMakeLists.txt

+install(TARGETS ${PROJECT_NAME}_lambda
+                ${PROJECT_NAME}_member_function
+                ${PROJECT_NAME}_not_composable
+        DESTINATION bin)


Please avoid aligned indentation.

OK. For posterity, I'll add some text to the style guide which makes this more clear.

dirk-thomas · 2016-11-29T15:22:14Z

rclcpp/minimal_publisher/CMakeLists.txt

+find_package(std_msgs REQUIRED)
+if (NOT WIN32)
+  set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++11 -Wall -Wextra")
+endif()


I would suggest moving the C++ flags above the find_package (as it is done in all other packages) since the CMake config files could already try to use them.

Thanks. I don't think it's done in all other packages though; I started this CMakeLists.txt file by copying from somewhere else. But I can't figure out now where I copied it from. Anyway I'll keep looking for other instances as well.

dirk-thomas · 2016-11-29T15:23:39Z

rclcpp/minimal_publisher/lambda.cpp

  {
-    publisher_ = this->create_publisher<std_msgs::msg::String>("chatter");
+    publisher_ = this->create_publisher<std_msgs::msg::String>("topic");


Why change the topic name? This will make the code incompatible with the ROS 1 equivalents.

That is true, but the ROS 1 equivalent has moved to http://github.com/ros2/tutorials where it will (eventually) have lots of inline comments and narrative documentation like in the ROS 1 code. The idea for this repository is to have lots of short copy-paste friendly examples that are as unambiguous as possible. Hence the name change from talker to minimal_publisher, etc.

dirk-thomas · 2016-11-29T15:25:01Z

Since the packages (intentionally) don't contain any automatic linting did you run all linters on the example code?

codebot · 2016-11-29T19:27:57Z

I'm trying to figure out how to run the linters manually. When I figure that out, I'll document it in the style guide or somewhere else.

dirk-thomas · 2016-11-29T19:32:50Z

For each linter you will find the way to invoke it in its documentation (e.g. https://github.com/ament/ament_lint/blob/master/ament_pep8/doc/index.rst).

codebot · 2016-11-29T22:52:36Z

OK everything is now using class instead of struct and the style checks and linters seem to be all passing now. Anything else before this merges?

dirk-thomas · 2016-11-29T22:59:18Z

Are additional rclcpp examples planned for a future PR? The ones in this PR look fine to me.

codebot · 2016-11-29T23:45:57Z

Yeah, I'm working on cleaning up some services examples which I'll put in a different PR.

dirk-thomas · 2016-11-29T23:48:20Z

Maybe we should also have examples how we recommend users to write their nodes in the future, inheriting from node and creating a library.

codebot · 2016-11-29T23:49:31Z

Yes that's a great idea. I'll make an issue for that on this repo.

dirk-thomas · 2016-11-30T01:41:10Z

Please squash your commits before / during the merge.

codebot · 2016-11-30T01:42:12Z

whoops. Is it too late now?

…

On Tue, Nov 29, 2016 at 5:41 PM, Dirk Thomas ***@***.***> wrote: Please squash your commits before / during the merge. — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#137 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFBR2i76aoX23jrLwoAdZcYhkZT7AIS6ks5rDNQ2gaJpZM4K6CYd> .

wjwwood · 2016-11-30T01:44:27Z

Technically no, but we shouldn't make a habit of force pushing to master. I'd say just leave it and try to remember in the future 😄.

codebot · 2016-11-30T01:45:32Z

ok thanks. Will do.

…

On Tue, Nov 29, 2016 at 5:44 PM, William Woodall ***@***.***> wrote: Technically no, but we shouldn't make a habit of force pushing to master. I'd say just leave it and try to remember in the future 😄. — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#137 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFBR2lQ64oFEivcSnxwwmz78c-GiS3bOks5rDNT7gaJpZM4K6CYd> .

add examples_ prefix to package names to avoid future collisions

402932f

codebot added the in progress Actively being worked on (Kanban column) label Nov 22, 2016

change talker/listener to minimal_publisher/minimal_subscriber

4fc7dd4

codebot mentioned this pull request Nov 23, 2016

Minimal service and client #138

Merged

codebot added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Nov 23, 2016

codebot self-assigned this Nov 23, 2016

mikaelarguedas reviewed Nov 29, 2016

View reviewed changes

mikaelarguedas approved these changes Nov 29, 2016

View reviewed changes

dirk-thomas reviewed Nov 29, 2016

View reviewed changes

codebot added 2 commits November 29, 2016 09:50

fix cmake indentation and c++11 flag ordering

234a9a2

attempt to improve indentation which includes a c++11 lambda

8818bdb

passing all linters!

1725f46

codebot merged commit 46adb61 into master Nov 30, 2016

codebot removed the in review Waiting for review (Kanban column) label Nov 30, 2016

codebot deleted the prefix_package_names branch November 30, 2016 01:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add examples_ prefix to package names to avoid future collisions #137

add examples_ prefix to package names to avoid future collisions #137

codebot commented Nov 22, 2016

mikaelarguedas commented Nov 22, 2016

codebot commented Nov 22, 2016

codebot commented Nov 22, 2016

mikaelarguedas commented Nov 22, 2016

codebot commented Nov 22, 2016

codebot commented Nov 23, 2016

mikaelarguedas Nov 29, 2016

mikaelarguedas left a comment

codebot commented Nov 29, 2016

dirk-thomas Nov 29, 2016

codebot Nov 29, 2016

dirk-thomas Nov 29, 2016

codebot Nov 29, 2016

dirk-thomas Nov 29, 2016

codebot Nov 29, 2016

dirk-thomas commented Nov 29, 2016

codebot commented Nov 29, 2016

dirk-thomas commented Nov 29, 2016

codebot commented Nov 29, 2016

dirk-thomas commented Nov 29, 2016

codebot commented Nov 29, 2016

dirk-thomas commented Nov 29, 2016

codebot commented Nov 29, 2016

dirk-thomas commented Nov 30, 2016

codebot commented Nov 30, 2016 via email

wjwwood commented Nov 30, 2016

codebot commented Nov 30, 2016 via email

add examples_ prefix to package names to avoid future collisions #137

add examples_ prefix to package names to avoid future collisions #137

Conversation

codebot commented Nov 22, 2016

mikaelarguedas commented Nov 22, 2016

codebot commented Nov 22, 2016

codebot commented Nov 22, 2016

mikaelarguedas commented Nov 22, 2016

codebot commented Nov 22, 2016

codebot commented Nov 23, 2016

mikaelarguedas Nov 29, 2016

Choose a reason for hiding this comment

mikaelarguedas left a comment

Choose a reason for hiding this comment

codebot commented Nov 29, 2016

dirk-thomas Nov 29, 2016

Choose a reason for hiding this comment

codebot Nov 29, 2016

Choose a reason for hiding this comment

dirk-thomas Nov 29, 2016

Choose a reason for hiding this comment

codebot Nov 29, 2016

Choose a reason for hiding this comment

dirk-thomas Nov 29, 2016

Choose a reason for hiding this comment

codebot Nov 29, 2016

Choose a reason for hiding this comment

dirk-thomas commented Nov 29, 2016

codebot commented Nov 29, 2016

dirk-thomas commented Nov 29, 2016

codebot commented Nov 29, 2016

dirk-thomas commented Nov 29, 2016

codebot commented Nov 29, 2016

dirk-thomas commented Nov 29, 2016

codebot commented Nov 29, 2016

dirk-thomas commented Nov 30, 2016

codebot commented Nov 30, 2016 via email

wjwwood commented Nov 30, 2016

codebot commented Nov 30, 2016 via email