Adding a get_slot_fields_and_types method to python msg classes #19
Conversation
| @[for field in spec.fields]@ | ||
| @[ if field.type.is_primitive_type() ]@ | ||
| @[ if field.type.is_array ]@ | ||
| '@(get_python_type(field.type))[@(field.type.array_size)]', |
There was a problem hiding this comment.
This doesn't distinguish between a static array (with a fixed size) and a dynamic array (either with an upper bound or without a bound).
There was a problem hiding this comment.
True. Do you have a preferred method to denote the different types of arrays? I didn't have a strong intuition here
There was a problem hiding this comment.
I would suggest either the same notation as in the .msg or an object representation. Thinking about it the second one is probably be better in terms of stability with the upcoming change to .idl files since the notation will change (see https://github.com/ros2/design/blob/gh-pages/articles/142_idl.md).
There was a problem hiding this comment.
Looking at the patch I don't see where the type is being distinguished?
There was a problem hiding this comment.
My apologies, I got my branches mixed up. All of my changes are in one commit (9454002) but I'm unsure of whether I should cherry pick that commit on top of master or 0.5.2. Since this is targeting master, I assume that I should cherry pick on top of that, but doing so causes the build to fail for unrelated reasons..
/home/mike/ws_rqt2/build/rosidl_generator_py/rosidl_generator_py/rosidl_generator_py/msg/_string_arrays_s.c:12:10: fatal error: rosidl_generator_c/primitives_sequence.h: No such file or directory #include <rosidl_generator_c/primitives_sequence.h> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ compilation terminated. /home/mike/ws_rqt2/build/rosidl_generator_py/rosidl_generator_py/rosidl_generator_py/msg/_nested_s.c:12:10: fatal error: rosidl_generator_c/primitives_sequence.h: No such file or directory #include <rosidl_generator_c/primitives_sequence.h> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ compilation terminated. /home/mike/ws_rqt2/build/rosidl_generator_py/rosidl_generator_py/rosidl_generator_py/msg/_various_s.c:12:10: fatal error: rosidl_generator_c/primitives_sequence.h: No such file or directory #include <rosidl_generator_c/primitives_sequence.h>
Is there a .repos available with the most recent ros2 branches that I should use for making PR's to master?
There was a problem hiding this comment.
It depends on what you are trying to build. I would suggest using the .repos files from the master branch in the ros2/ros2 repo and building everything from source for your use case. Then a patch based on the master of this repo will work.
(The opposite would be to use the bouncy branch of the .repos file and then also target the bouncy branch in this repo. But usually all contributions are targeting the default branch and only in case of severe bug fixes are being considered for backporting afterwards.)
There was a problem hiding this comment.
The current state seems to pass on CI:
mlautman
force-pushed
the
adding-slot-types-to-python-msgs
branch
2 times, most recently
from
7ed6394
to
47cf04c
Compare
|
@dirk-thomas ready for another review |
mlautman
force-pushed
the
adding-slot-types-to-python-msgs
branch
from
47cf04c
to
9454002
Compare
mlautman
force-pushed
the
adding-slot-types-to-python-msgs
branch
2 times, most recently
from
068fe49
to
85aa7b7
Compare
|
@dirk-thomas Changed |
|
Ignoring W504 all over the place isn't necessary. This has been addressed in the linter configuration already a while ago: ament/ament_lint#110. |
|
@dirk-thomas It still fails locally with |
Are you using the latest default branches of all the other ROS 2 repos? |
| @@ -175,6 +197,10 @@ class @(spec.base_type.type)(metaclass=Metaclass): | |||
| return False | |||
| @[end for]@ | |||
| return True | |||
|
|
|||
| @@classmethod | |||
| def get_slot_types_dict(cls): | |||
There was a problem hiding this comment.
| def get_slot_types_dict(cls): | |
| def get_slot_fields_and_types(cls): |
|
|
||
| @@classmethod | ||
| def get_fields_and_field_types(cls): | ||
| return cls._slot_types_dict |
There was a problem hiding this comment.
Since this is a getter, should it return a shallow copy so that you can modify the underlying dictionary?
There was a problem hiding this comment.
Good catch. Fixed.
mlautman
force-pushed
the
adding-slot-types-to-python-msgs
branch
2 times, most recently
from
e820439
to
ac9c660
Compare
mlautman
changed the title
mlautman
force-pushed
the
adding-slot-types-to-python-msgs
branch
from
ac9c660
to
40d4bcc
Compare
|
@dirk-thomas I am using the ros2.repos that you passed along to us with the 0.5.2 release. I went back and removed W504. I also changed the code to return a copy of the dict rather than the dict itself per @brawner's comment |
mlautman
force-pushed
the
adding-slot-types-to-python-msgs
branch
from
343ce69
to
706a5dd
Compare
|
This can be merged as is for now. With the coming change I will likely change this in the following way:
|
This adds a
_slot_typesfield to python message classes as was requested here: #16This attempts to remain as true to the ROS1 functionality as possible