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

Relocate to_yaml() under message namespace. #609

Merged
merged 1 commit into from
Aug 4, 2021

Conversation

hidmic
Copy link
Contributor

@hidmic hidmic commented Aug 3, 2021

Closes #607. This enables argument dependent lookup. to_yaml() overloads under the rosidl_generator_traits namespace were kept but deprecated.

CI up to rosidl_generator_cpp (to start with):

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

This enables argument dependent lookups.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Copy link
Contributor Author

hidmic commented Aug 4, 2021

Full CI, to be safe:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status (unrelated test failures)

@@ -84,14 +87,14 @@ inline void to_yaml(
@[ if isinstance(member.type, BasicType)]@
out << "@(member.name): ";
@[ if member.type.typename in ('octet', 'char', 'wchar')]@
character_value_to_yaml(msg.@(member.name), out);
rosidl_generator_traits::character_value_to_yaml(msg.@(member.name), out);
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if we should keep character_value_to_yaml() in rosidl_generator_traits or if we should move it to another place

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, they have to be in a common place. I don't mind where. I figured keeping them where they are was the path of least resistance, but I'm open to ideas.

Copy link
Member

Choose a reason for hiding this comment

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

sounds good.
rosidl_generator_traits doesn't sound like the namespace for this helper functions, but it doesn't really matter.

@hidmic hidmic requested a review from ivanpauno August 4, 2021 18:27
@hidmic
Copy link
Contributor Author

hidmic commented Aug 4, 2021

Alright, going in!

@hidmic hidmic merged commit a1eb6ed into master Aug 4, 2021
@delete-merged-branch delete-merged-branch bot deleted the hidmic/adl-enabled-to_yaml branch August 4, 2021 21:18
@ros-discourse
Copy link

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros-2-type-support-introspection-and-conversion-of-c-c-messages-to-from-yaml/22295/1

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.

Relocate to_yaml() definition to message namespace
4 participants