simplify code using updated definition API #45
Conversation
Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
| @@ -180,12 +178,7 @@ def constant_value_to_py(type_, value): | |||
| if type_.typename == 'bool': | |||
There was a problem hiding this comment.
@dirk-thomas hmm shouldn't it be 'boolean'? Also, here and everywhere else, why not use BOOLEAN_TYPE?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| @@ -194,7 +187,7 @@ def constant_value_to_py(type_, value): | |||
| if type_.typename == 'octet': | |||
There was a problem hiding this comment.
@dirk-thomas same about OCTET_TYPE
| 'int32', 'uint32', | ||
| 'int64', 'uint64', | ||
| ): | ||
| if type_.typename in INTEGER_TYPES: | ||
| return str(value) | ||
|
|
||
| if type_.typename == 'char': |
There was a problem hiding this comment.
@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).
There was a problem hiding this comment.
See #45 (comment). Why should we create a constant for that name (CHAR_TYPE = 'char')? What would be the advantage?
There was a problem hiding this comment.
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'.
delete-merged-branch
bot
deleted the
simplification-using-updated-definition-api
branch
Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
Connect to ros2/rosidl#364.