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 #4804

Open
wants to merge 1 commit into
base: jazzy
Choose a base branch
from

Conversation

EliottFerry
Copy link

Changing python codeblock to match previous AddThreeInts implementation (C++ might need a check also, i followed only the Python tutorial)

Changing python codeblock to match previous AddThreeInts implementation (C++ might need a check also, i followed only the Python tutorial)

Signed-off-by: EliottFerry <[email protected]>
@kscottz
Copy link
Collaborator

kscottz commented Oct 4, 2024

@EliottFerry can you clarify what you mean by, "match previous AddThreeInts implementation."

@EliottFerry
Copy link
Author

EliottFerry commented Oct 4, 2024

@kscottz Of course, on this step, you have the implementation of MinimalClientAsync along with a main function. This main implementation does not match with this implementation, where some error handling is being implemented, without the #CHANGE tags showing the line that as been changed.
I got "confused" reaching this step, since I did not understood why the main function had changed that much. Maybe the second implementation should be the one to be kept to give an example of error handling.

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.

Thanks for the contribution.

Can you please keep the indentation the same for the parts of the code that don't change? That will make this easier to review.

Also, CI is failing because we don't allow trailing whitespace. If you fix that, we can review this. Thanks.

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