Add types to rosidl_generator_type_description #840
Conversation
Signed-off-by: Michael Carlstrom <[email protected]>
InvincibleRMC
requested review from
adityapande-1995,
methylDragon,
quarkytale and
sloretz
as code owners
Signed-off-by: Michael Carlstrom <[email protected]>
Signed-off-by: Michael Carlstrom <[email protected]>
Signed-off-by: Michael Carlstrom <[email protected]>
Signed-off-by: Michael Carlstrom <[email protected]>
Signed-off-by: Michael Carlstrom <[email protected]>
|
@Mergifyio update |
✅ Branch has been successfully updated |
|
Pulls: #840
|
|
@sloretz can the CI be re-run since the windows bug has been fixed? |
|
@Mergifyio update |
☑️ Nothing to do
|
|
Pulls: #840
|
Signed-off-by: Michael Carlstrom <[email protected]>
|
Pulls: #840
|
|
@sloretz is this good to merge in? |
| if isinstance(value_type, definition.BasicType): | ||
| value_type_name = FIELD_VALUE_TYPE_NAMES[value_type.typename] | ||
| elif isinstance(value_type, definition.AbstractGenericString): | ||
| elif isinstance(value_type, FIELD_VALUE_STRING_TYPES): |
There was a problem hiding this comment.
I will point out that this is not quite the same thing as what it is replacing. In particular, while FIELD_VALUE_STRING_TYPES covers all of the currently defined AbstractGenericString types, it does not account for any types that might be derived in the future, or in user code. While I think those are unlikely, they are not impossible.
Can you explain a bit more why you did this change?
There was a problem hiding this comment.
Since this value is used to index into FIELD_VALUE_TYPE_NAMES. It has the more specific sub classes. If you wanted to add future string types you would need to update FIELD_VALUE_STRING_TYPES as well as FIELD_VALUE_TYPE_NAMES.
FIELD_VALUE_TYPE_NAMES: Final = {
None: 'FIELD_TYPE_NOT_SET',
'nested_type': 'FIELD_TYPE_NESTED_TYPE',
'int8': 'FIELD_TYPE_INT8',
'uint8': 'FIELD_TYPE_UINT8',
'int16': 'FIELD_TYPE_INT16',
'uint16': 'FIELD_TYPE_UINT16',
'int32': 'FIELD_TYPE_INT32',
'uint32': 'FIELD_TYPE_UINT32',
'int64': 'FIELD_TYPE_INT64',
'uint64': 'FIELD_TYPE_UINT64',
'float': 'FIELD_TYPE_FLOAT',
'double': 'FIELD_TYPE_DOUBLE',
'long': 'LONG_DOUBLE',
'char': 'FIELD_TYPE_CHAR',
'wchar': 'FIELD_TYPE_WCHAR',
'boolean': 'FIELD_TYPE_BOOLEAN',
'octet': 'FIELD_TYPE_BYTE',
definition.UnboundedString: 'FIELD_TYPE_STRING',
definition.UnboundedWString: 'FIELD_TYPE_WSTRING',
# NOTE: rosidl_parser does not define fixed string types
definition.BoundedString: 'FIELD_TYPE_BOUNDED_STRING',
definition.BoundedWString: 'FIELD_TYPE_BOUNDED_WSTRING',
}
rosidl_generator_type_description/rosidl_generator_type_description/__init__.py
Show resolved
Hide resolved
rosidl_generator_type_description/rosidl_generator_type_description/__init__.py
Outdated
Show resolved
Hide resolved
| annotation_value = member.get_annotation_value('default') | ||
| if isinstance(annotation_value, dict): | ||
| default_value = str(annotation_value['value']) |
There was a problem hiding this comment.
Again this isn't quite the same thing as what it is replacing. Down below, if member.has_annotation('default') is true, we'll always get the value and then try to convert to string. Here we only do it in certain circumstances, which are valid, but may not be all of the circumstances. Can you explain a bit more about this one?
There was a problem hiding this comment.
member.get_annotation_value('default') can return Union[str, int, float, bool, Dict[str, Union[str, int, float, bool]], None]. The only type that can be indexed into with ['value'] is the dictionary. So I added the isinstance(annotation_value, dict) check.
Signed-off-by: Michael Carlstrom <[email protected]>
3 participants
No description provided.