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

Implement test for nesting types and nesting optional types #17

Closed
asmodehn opened this issue Apr 26, 2017 · 4 comments
Closed

Implement test for nesting types and nesting optional types #17

asmodehn opened this issue Apr 26, 2017 · 4 comments
Assignees

Comments

@asmodehn
Copy link
Member

We currently test only different field type.
Therefore we never test the recursion of a type inside a type inside a type, etc.
But nesting types is supposed to work (it is the way to combine message type in ROS and must be supported).

This is not tested yet.
A good first step would be to add test for all message types in std_msgs.

  • what if they are optional inside another message ?
  • what if the fields they contain are optional ?
@asmodehn
Copy link
Member Author

A first iteration has been done in #14
Not ideal though since one needs to specify dynamically which fields are optionals... It should be static, in the message definition itself... Maybe using constants ?

@asmodehn asmodehn self-assigned this Sep 4, 2017
@asmodehn
Copy link
Member Author

asmodehn commented Sep 5, 2017

It seems that in all cases the user has to dynamically patch the python code generated for the message.
This is already the case with opt_as_array which has a nice message definition syntax.

There were original two main goals for opt_as_nested as improvement over opt_as_array:

  1. the optional field is setup statically in the message definition, as it also helps the user seeing the definition as ROS API documentation
  2. the optional field is marked as initialized dynamically (by some custom code) so the receiver can deduce the appropriate value to use when deserializing.

The current implementation doesnt fullfil 1. since the patch has to be setup dynamically in any user code...
Another problem is that the client still need to do dynamic patching and declare which field is optional...

@asmodehn
Copy link
Member Author

asmodehn commented Sep 5, 2017

I wasn't able to find any obvious improvement or "best" solution for this problem, so the best strategy now is to wait until a real strong usecase for opt_as_nested appears, and then decide by experiments which implementation works best.

The current implementation, although not perfect, is still usable working fine.

@asmodehn
Copy link
Member Author

closing as the PR has been merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant