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.

Sign up for GitHub

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

Jump to bottom

simplify code using updated definition API #45

Merged
merged 1 commit into from
Apr 22, 2019
Merged

simplify code using updated definition API #45

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion rosidl_generator_py/resource/_action.py.em
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ class Metaclass_@(action.namespaced_type.name)(type):
import logging
import traceback
logger = logging.getLogger(
'@('.'.join(action.namespaced_type.namespaces + [action.namespaced_type.name]))')
'@('.'.join(action.namespaced_type.namespaced_name()))')
logger.debug(
'Failed to import needed modules for type support:\n' +
traceback.format_exc())
Expand Down
2 changes: 1 addition & 1 deletion rosidl_generator_py/resource/_action_pkg_typesupport_entry_point.c.em
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ int8_t
int8_t err;
PyObject * pyobject_@(function_name) = NULL;
pyobject_@(function_name) = PyCapsule_New(
(void *)ROSIDL_TYPESUPPORT_INTERFACE__ACTION_SYMBOL_NAME(rosidl_typesupport_c, @(', '.join(action.namespaced_type.namespaces + [action.namespaced_type.name])))(),
(void *)ROSIDL_TYPESUPPORT_INTERFACE__ACTION_SYMBOL_NAME(rosidl_typesupport_c, @(', '.join(action.namespaced_type.namespaced_name())))(),
NULL, NULL);
if (!pyobject_@(function_name)) {
// previously added objects will be removed when the module is destroyed
Expand Down
39 changes: 19 additions & 20 deletions rosidl_generator_py/resource/_msg.py.em
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,14 @@ from rosidl_parser.definition import ACTION_GOAL_SUFFIX
from rosidl_parser.definition import ACTION_RESULT_SUFFIX
from rosidl_parser.definition import Array
from rosidl_parser.definition import BasicType
from rosidl_parser.definition import BOOLEAN_TYPE
from rosidl_parser.definition import BoundedSequence
from rosidl_parser.definition import CHARACTER_TYPES
from rosidl_parser.definition import FLOATING_POINT_TYPES
from rosidl_parser.definition import INTEGER_TYPES
from rosidl_parser.definition import NamespacedType
from rosidl_parser.definition import SIGNED_INTEGER_TYPES
from rosidl_parser.definition import UNSIGNED_INTEGER_TYPES
}@
@#<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
@# Collect necessary import statements for all members
Expand Down Expand Up @@ -91,7 +97,7 @@ class Metaclass_@(message.structure.namespaced_type.name)(type):
import logging
import traceback
logger = logging.getLogger(
'@('.'.join(message.structure.namespaced_type.namespaces + [message.structure.namespaced_type.name]))')
'@('.'.join(message.structure.namespaced_type.namespaced_name()))')
logger.debug(
'Failed to import needed modules for type support:\n' +
traceback.format_exc())
Expand Down Expand Up @@ -253,7 +259,7 @@ if isinstance(type_, AbstractNestedType):
'@(member.name)',
[bytes([0]) for x in range(@(member.type.size))]
)
@[ elif isinstance(type_, BasicType) and type_.typename in ('char', 'wchar')]@
@[ elif isinstance(type_, BasicType) and type_.typename in CHARACTER_TYPES]@
self.@(member.name) = kwargs.get(
'@(member.name)',
[chr(0) for x in range(@(member.type.size))]
Expand All @@ -280,7 +286,7 @@ if isinstance(type_, AbstractNestedType):
@[ end if]@
@[ elif isinstance(type_, BasicType) and type_.typename == 'octet']@
self.@(member.name) = kwargs.get('@(member.name)', bytes([0]))
@[ elif isinstance(type_, BasicType) and type_.typename in ('char', 'wchar')]@
@[ elif isinstance(type_, BasicType) and type_.typename in CHARACTER_TYPES]@
self.@(member.name) = kwargs.get('@(member.name)', chr(0))
@[ else]@
self.@(member.name) = kwargs.get('@(member.name)', @(get_python_type(type_))())
Expand Down Expand Up @@ -374,7 +380,7 @@ if member.name in dict(inspect.getmembers(builtins)).keys():
from collections import UserString
@[ elif isinstance(type_, BasicType) and type_.typename == 'octet']@
from collections.abc import ByteString
@[ elif isinstance(type_, BasicType) and type_.typename in ('char', 'wchar')]@
@[ elif isinstance(type_, BasicType) and type_.typename in CHARACTER_TYPES]@
from collections import UserString
@[ end if]@
assert \
Expand All @@ -389,7 +395,7 @@ if member.name in dict(inspect.getmembers(builtins)).keys():
all(len(val) <= @(type_.maximum_size) for val in value) and
@{assert_msg_suffixes.append('and each string value not longer than %d' % type_.maximum_size)}@
@[ end if]@
@[ if isinstance(member.type, Array) or isinstance(member.type, BoundedSequence)]@
@[ if isinstance(member.type, (Array, BoundedSequence))]@
@[ if isinstance(member.type, BoundedSequence)]@
len(value) <= @(member.type.maximum_size) and
@{assert_msg_suffixes.insert(1, 'with length <= %d' % member.type.maximum_size)}@
Expand All @@ -400,14 +406,14 @@ if member.name in dict(inspect.getmembers(builtins)).keys():
@[ end if]@
all(isinstance(v, @(get_python_type(type_))) for v in value) and
@{assert_msg_suffixes.append("and each value of type '%s'" % get_python_type(type_))}@
@[ if isinstance(type_, BasicType) and type_.typename.startswith('int')]@
@[ if isinstance(type_, BasicType) and type_.typename in SIGNED_INTEGER_TYPES]@
@{
nbits = int(type_.typename[3:])
bound = 2**(nbits - 1)
}@
all(val >= -@(bound) and val < @(bound) for val in value)), \
@{assert_msg_suffixes.append('and each integer in [%d, %d]' % (-bound, bound - 1))}@
@[ elif isinstance(type_, BasicType) and type_.typename.startswith('uint')]@
@[ elif isinstance(type_, BasicType) and type_.typename in UNSIGNED_INTEGER_TYPES]@
@{
nbits = int(type_.typename[4:])
bound = 2**nbits
Expand All @@ -422,43 +428,36 @@ bound = 2**nbits
@[ end if]@
"The '@(member.name)' field must be @(' '.join(assert_msg_suffixes))"
@[ elif isinstance(member.type, AbstractGenericString) and member.type.has_maximum_size()]@
((isinstance(value, str) or isinstance(value, UserString)) and
(isinstance(value, (str, UserString)) and
len(value) <= @(member.type.maximum_size)), \
"The '@(member.name)' field must be string value " \
'not longer than @(type_.maximum_size)'
@[ elif isinstance(type_, NamespacedType)]@
isinstance(value, @(type_.name)), \
"The '@(member.name)' field must be a sub message of type '@(type_.name)'"
@[ elif isinstance(type_, BasicType) and type_.typename == 'octet']@
((isinstance(value, bytes) or isinstance(value, ByteString)) and
(isinstance(value, (bytes, ByteString)) and
len(value) == 1), \
"The '@(member.name)' field must be of type 'bytes' or 'ByteString' with length 1"
@[ elif isinstance(type_, BasicType) and type_.typename == 'char']@
((isinstance(value, str) or isinstance(value, UserString)) and
(isinstance(value, (str, UserString)) and
len(value) == 1 and ord(value) >= -128 and ord(value) < 128), \
"The '@(member.name)' field must be of type 'str' or 'UserString' " \
'with length 1 and the character ord() in [-128, 127]'
@[ elif isinstance(type_, AbstractGenericString)]@
isinstance(value, str), \
"The '@(member.name)' field must be of type '@(get_python_type(type_))'"
@[ elif isinstance(type_, BasicType) and type_.typename in [
'boolean',
'float', 'double',
'int8', 'uint8',
'int16', 'uint16',
'int32', 'uint32',
'int64', 'uint64',
]]@
@[ elif isinstance(type_, BasicType) and type_.typename in (BOOLEAN_TYPE, *FLOATING_POINT_TYPES, *INTEGER_TYPES)]@
isinstance(value, @(get_python_type(type_))), \
"The '@(member.name)' field must be of type '@(get_python_type(type_))'"
@[ if type_.typename.startswith('int')]@
@[ if type_.typename in SIGNED_INTEGER_TYPES]@
@{
nbits = int(type_.typename[3:])
bound = 2**(nbits - 1)
}@
assert value >= -@(bound) and value < @(bound), \
"The '@(member.name)' field must be an integer in [@(-bound), @(bound - 1)]"
@[ elif type_.typename.startswith('uint')]@
@[ elif type_.typename in UNSIGNED_INTEGER_TYPES]@
@{
nbits = int(type_.typename[4:])
bound = 2**nbits
Expand Down
6 changes: 3 additions & 3 deletions rosidl_generator_py/resource/_msg_pkg_typesupport_entry_point.c.em
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ header_files = [
@
@{
module_name = convert_camel_case_to_lower_case_underscore(message.structure.namespaced_type.name)
msg_typename = '__'.join(message.structure.namespaced_type.namespaces + [message.structure.namespaced_type.name])
msg_typename = '__'.join(message.structure.namespaced_type.namespaced_name())
}@

static void * @('__'.join(message.structure.namespaced_type.namespaces + [module_name]))__create_ros_message(void)
Expand All @@ -56,7 +56,7 @@ PyObject * @('__'.join(message.structure.namespaced_type.namespaces + [module_na

ROSIDL_GENERATOR_C_IMPORT
const rosidl_message_type_support_t *
ROSIDL_GET_MSG_TYPE_SUPPORT(@(', '.join(message.structure.namespaced_type.namespaces + [message.structure.namespaced_type.name])));
ROSIDL_GET_MSG_TYPE_SUPPORT(@(', '.join(message.structure.namespaced_type.namespaced_name())));

@{
register_function = '_register_msg_type__' + '__'.join(message.structure.namespaced_type.namespaces[1:] + [module_name])
Expand All @@ -76,7 +76,7 @@ function_names = ['create_ros_message', 'destroy_ros_message', 'convert_from_py'
@[ if function_name != 'type_support']@
(void *)&@('__'.join(message.structure.namespaced_type.namespaces + [module_name]))__@(function_name),
@[ else]@
(void *)ROSIDL_GET_MSG_TYPE_SUPPORT(@(', '.join(message.structure.namespaced_type.namespaces + [message.structure.namespaced_type.name]))),
(void *)ROSIDL_GET_MSG_TYPE_SUPPORT(@(', '.join(message.structure.namespaced_type.namespaced_name()))),
@[ end if]@
NULL, NULL);
if (!pyobject_@(function_name)) {
Expand Down
6 changes: 3 additions & 3 deletions rosidl_generator_py/resource/_msg_support.c.em
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ if isinstance(member.type, AbstractNestedType) and isinstance(member.type.value_
// end nested array functions include
@[end if]@
@{
msg_typename = '__'.join(message.structure.namespaced_type.namespaces + [message.structure.namespaced_type.name])
msg_typename = '__'.join(message.structure.namespaced_type.namespaced_name())
}@
@
@[for member in message.structure.members]@
Expand Down Expand Up @@ -191,7 +191,7 @@ if isinstance(type_, AbstractNestedType):
}
@[ if isinstance(type_, NamespacedType)]@
@{
nested_type = '__'.join(type_.namespaces + [type_.name])
nested_type = '__'.join(type_.namespaced_name())
}@
@[ if isinstance(member.type, AbstractNestedType)]@
PyObject * seq_field = PySequence_Fast(field, "expected a sequence in '@(member.name)'");
Expand Down Expand Up @@ -496,7 +496,7 @@ if isinstance(type_, AbstractNestedType):
@[ else]@
@[ if isinstance(type_, NamespacedType)]@
@{
nested_type = '__'.join(type_.namespaces + [type_.name])
nested_type = '__'.join(type_.namespaced_name())
}@
@[ if isinstance(member.type, AbstractNestedType)]@
@[ if isinstance(member.type, AbstractSequence)]@
Expand Down
2 changes: 1 addition & 1 deletion rosidl_generator_py/resource/_srv.py.em
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ class Metaclass_@(service.namespaced_type.name)(type):
import logging
import traceback
logger = logging.getLogger(
'@('.'.join(service.namespaced_type.namespaces + [service.namespaced_type.name]))')
'@('.'.join(service.namespaced_type.namespaced_name()))')
logger.debug(
'Failed to import needed modules for type support:\n' +
traceback.format_exc())
Expand Down
4 changes: 2 additions & 2 deletions rosidl_generator_py/resource/_srv_pkg_typesupport_entry_point.c.em
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ function_name = 'type_support'

ROSIDL_GENERATOR_C_IMPORT
const rosidl_service_type_support_t *
ROSIDL_TYPESUPPORT_INTERFACE__SERVICE_SYMBOL_NAME(rosidl_typesupport_c, @(', '.join(service.namespaced_type.namespaces + [service.namespaced_type.name])))();
ROSIDL_TYPESUPPORT_INTERFACE__SERVICE_SYMBOL_NAME(rosidl_typesupport_c, @(', '.join(service.namespaced_type.namespaced_name())))();

@{
register_function = '_register_srv_type__' + '__'.join(service.namespaced_type.namespaces[1:] + [type_name])
Expand All @@ -37,7 +37,7 @@ int8_t
int8_t err;
PyObject * pyobject_@(function_name) = NULL;
pyobject_@(function_name) = PyCapsule_New(
(void *)ROSIDL_TYPESUPPORT_INTERFACE__SERVICE_SYMBOL_NAME(rosidl_typesupport_c, @(', '.join(service.namespaced_type.namespaces + [service.namespaced_type.name])))(),
(void *)ROSIDL_TYPESUPPORT_INTERFACE__SERVICE_SYMBOL_NAME(rosidl_typesupport_c, @(', '.join(service.namespaced_type.namespaced_name())))(),
NULL, NULL);
if (!pyobject_@(function_name)) {
// previously added objects will be removed when the module is destroyed
Expand Down
38 changes: 11 additions & 27 deletions rosidl_generator_py/rosidl_generator_py/generate_py_impl.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,11 @@
from rosidl_parser.definition import AbstractString
from rosidl_parser.definition import Array
from rosidl_parser.definition import BasicType
from rosidl_parser.definition import CHARACTER_TYPES
from rosidl_parser.definition import FLOATING_POINT_TYPES
from rosidl_parser.definition import IdlContent
from rosidl_parser.definition import IdlLocator
from rosidl_parser.definition import INTEGER_TYPES
from rosidl_parser.definition import NamespacedType
from rosidl_parser.parser import parse_idl_file

Expand Down Expand Up @@ -153,12 +156,7 @@ def primitive_value_to_py(type_, value):
if type_.typename == 'boolean':
return 'True' if value else 'False'

if type_.typename in (
'int8', 'uint8',
'int16', 'uint16',
'int32', 'uint32',
'int64', 'uint64',
):
if type_.typename in INTEGER_TYPES:
return str(value)

if type_.typename == 'char':
Expand All @@ -167,7 +165,7 @@ def primitive_value_to_py(type_, value):
if type_.typename == 'octet':
return repr(bytes([value]))

if type_.typename in ('float', 'double'):
if type_.typename in FLOATING_POINT_TYPES:
return '%s' % value

assert False, "unknown primitive type '%s'" % type_.typename
Expand All @@ -180,12 +178,7 @@ def constant_value_to_py(type_, value):
if type_.typename == 'bool':
Copy link
Contributor

Choose a reason for hiding this comment

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

@dirk-thomas hmm shouldn't it be 'boolean'? Also, here and everywhere else, why not use BOOLEAN_TYPE?

Copy link
Member Author

Choose a reason for hiding this comment

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

For a single typename it doesn't really make a difference. In cases where the constant represents a set of types it makes sense to use the constant in case the set changes later as well as for readability.

Since there are 100+ cases where single typenames are used as string literals in the various generators I would rather not touch it in this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

The linter plugins can catch misspellings of BOOLEAN_TYPE, but not misspellings of 'bool'. I don't think it needs to be done in this PR, just pointing out there is a benefit.

Copy link
Contributor

Choose a reason for hiding this comment

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

I do see the improvement in readability, but if the sets were to ever change, having these variables won't do much against the other 100+ cases were the raw string is used (unless it's pure extension, of course).

In any case, above shouldn't it be 'boolean' comment still applies, or otherwise I don't follow why 'boolean' is used everywhere else when checking BasicType.typename attributes.

Copy link
Member Author

@dirk-thomas dirk-thomas Apr 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

In any case, above shouldn't it be 'boolean' comment still applies

Absolutely, obviously we currently don't have any boolean constants 😉. Adding one make the code fall through to the exception. I created a follow PR to fix the behavior (since this doesn't aim to change any behavior): #46.

return 'True' if value else 'False'

if type_.typename in (
'int8', 'uint8',
'int16', 'uint16',
'int32', 'uint32',
'int64', 'uint64',
):
if type_.typename in INTEGER_TYPES:
return str(value)

if type_.typename == 'char':
Copy link
Contributor

Choose a reason for hiding this comment

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

@dirk-thomas same about an equivalent CHAR_TYPE (I know there's none, I just find it awkward to use the variable in some places and the raw string in others).

Copy link
Member Author

Choose a reason for hiding this comment

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

See #45 (comment). Why should we create a constant for that name (CHAR_TYPE = 'char')? What would be the advantage?

Copy link
Member Author

@dirk-thomas dirk-thomas Apr 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Just copying a comment from @sloretz from above which applies here:

The linter plugins can catch misspellings of BOOLEAN_TYPE, but not misspellings of 'bool'.

Expand All @@ -194,7 +187,7 @@ def constant_value_to_py(type_, value):
if type_.typename == 'octet':
Copy link
Contributor

Choose a reason for hiding this comment

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

@dirk-thomas same about OCTET_TYPE

return repr(bytes([value]))

if type_.typename in ('float', 'double'):
if type_.typename in FLOATING_POINT_TYPES:
return '%s' % value

if isinstance(type_, AbstractString):
Expand Down Expand Up @@ -228,7 +221,7 @@ def get_python_type(type_):

if (
isinstance(type_.value_type, BasicType) and
type_.value_type.typename in ('char', 'wchar')
type_.value_type.typename in CHARACTER_TYPES
):
return 'str'

Expand All @@ -238,22 +231,13 @@ def get_python_type(type_):
if isinstance(type_, BasicType) and type_.typename == 'octet':
return 'bytes'

if isinstance(type_, BasicType) and type_.typename in (
'int8', 'uint8',
'int16', 'uint16',
'int32', 'uint32',
'int64', 'uint64',
):
if isinstance(type_, BasicType) and type_.typename in INTEGER_TYPES:
return 'int'

if isinstance(type_, BasicType) and type_.typename in (
'float', 'double',
):
if isinstance(type_, BasicType) and type_.typename in FLOATING_POINT_TYPES:
return 'float'

if isinstance(type_, BasicType) and type_.typename in (
'char', 'wchar',
):
if isinstance(type_, BasicType) and type_.typename in CHARACTER_TYPES:
return 'str'

assert False, "unknown type '%s'" % type_
8 changes: 3 additions & 5 deletions rosidl_runtime_py/rosidl_runtime_py/convert.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,9 +128,7 @@ def _convert_value(value, truncate_length=None):
elif isinstance(value, str):
if truncate_length is not None and len(value) > truncate_length:
value = value[:truncate_length] + '...'
elif (any(
isinstance(value, t) for t in [list, tuple, array.array, numpy.ndarray]
)):
elif isinstance(value, (list, tuple, array.array, numpy.ndarray)):
# Since arrays and ndarrays can't contain mixed types convert to list
typename = tuple if isinstance(value, tuple) else list
if truncate_length is not None and len(value) > truncate_length:
Expand All @@ -143,7 +141,7 @@ def _convert_value(value, truncate_length=None):
# Truncate every item in the list
value = typename(
[_convert_value(v, truncate_length) for v in value])
elif isinstance(value, dict) or isinstance(value, OrderedDict):
elif isinstance(value, (dict, OrderedDict)):
# Convert each key and value in the mapping
new_value = {} if isinstance(value, dict) else OrderedDict()
for k, v in value.items():
Expand All @@ -152,7 +150,7 @@ def _convert_value(value, truncate_length=None):
value = new_value
elif isinstance(value, numpy.number):
value = value.item()
elif not any(isinstance(value, t) for t in (bool, float, int)):
elif not isinstance(value, (bool, float, int)):
# Assuming value is a message since it is neither a collection nor a primitive type
value = message_to_ordereddict(value, truncate_length=truncate_length)
return value