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

Update Custom-ROS2-Interfaces.rst #4229

Closed
wants to merge 1 commit into from

Conversation

followthwhiterabbit
Copy link
Contributor

I have changed the executable file's name inside add_executable in CmakeLists.txt file. Because the files were from the previous sum for 2 arguments from the console but the files we updated were designed for three items.

I have changed the executable file's name inside add_executable in CmakeLists.txt file. Because the files were from the previous sum for 2 arguments from the console but the files we updated were designed for three items. 

Signed-off-by: followthwhiterabbit <[email protected]>
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

I see what you are saying about this being a bit confusing, but if we are going to do this we are going to have to add additional text as well. In particular, we are going to have to tell the user to rename their files from add_two_ints_server.cpp and add_two_ints_client.cpp (what they created in previous tutorials) to add_three_ints_server.cpp and add_three_ints_client.cpp, respectively.

I'm honestly not sure that is worth it. It is going to add more text, for a minor increase in understanding.

@followthwhiterabbit
Copy link
Contributor Author

It may not be worth it, yes, you're right, I am following the tutorials and had a build error problem, most of the people who follow the documentation should have found the solution already, I am just being meticulous

Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

most of the people who follow the documentation should have found the solution already

maybe.

but as tutorial, this PR fixes right, and adding the sentence like you need to change the file names ... seems to be the right thing here as @clalancette suggested.

@clalancette
Copy link
Contributor

I'm going to close this one out for now, since I don't think we are going to merge this as-is. Thanks for the contribution, though.

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.

3 participants