Skip to content

Parsing of the message structure from introspection typesupport library #287

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

Closed
wants to merge 6 commits into from

Conversation

nnmm
Copy link
Contributor

@nnmm nnmm commented Oct 27, 2022

This module provides a nice tree representation of the message structure.

@nnmm nnmm requested a review from a team October 27, 2022 17:54
jhdcs
jhdcs previously approved these changes Oct 27, 2022
Copy link
Collaborator

@jhdcs jhdcs left a comment

Choose a reason for hiding this comment

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

This looks all right to me. Thank you!

@nnmm
Copy link
Contributor Author

nnmm commented Oct 27, 2022

Wonderful @jhdcs, thanks for the quick review! I'll leave this open for one or two days to give @esteve a chance to see it too.

@esteve
Copy link
Collaborator

esteve commented Oct 27, 2022

@nnmm I won't be able to review this until next week or maybe even later. Given that this PR is not a priority, would you be ok with waiting until then? Thanks

@nnmm
Copy link
Contributor Author

nnmm commented Oct 28, 2022

@esteve Sure. I'll probably open one or more other PRs for this feature that you can batch review when you have time.

This module provides a nice tree representation of the message structure.
@nnmm
Copy link
Contributor Author

nnmm commented Nov 4, 2022

I forgot that I had written a test for this. Added now.

@nnmm nnmm force-pushed the dynamic_messages_3 branch from e468712 to e1efd85 Compare November 5, 2022 17:19
@esteve esteve self-requested a review November 10, 2022 14:53
@@ -1,5 +1,8 @@
#![cfg(test)]

mod client_service_tests;
// Disabled in Foxy due to https://github.com/ros2/rosidl/issues/598
#[cfg(all(not(ros_distro = "foxy"), not(ros_distro = "galactic")))]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why was this disabled for Galactic? It seemed that only Foxy was affected

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, actually also Galactic. I was wrong at first – should correct the comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

In that case, can you update the comment to mention galactic as well, instead of only foxy? Thanks

Copy link
Collaborator

Choose a reason for hiding this comment

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

@nnmm can you update the comment here? I can push the change, but given that this PR is part of a bigger PR, I thought of leaving it to you.

@nnmm
Copy link
Contributor Author

nnmm commented Nov 18, 2022

@esteve could you take another look please?

@esteve
Copy link
Collaborator

esteve commented Aug 15, 2024

@nnmm closing for now since we have had time to revisit this PR in a long time. Let us know if you'd like to work on it in the future. Thanks!

@esteve esteve closed this Aug 15, 2024
@esteve esteve deleted the dynamic_messages_3 branch August 29, 2024 13:19
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