-
Notifications
You must be signed in to change notification settings - Fork 51
Added forced casting of int values to float for float-typed fields #216
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
base: rolling
Are you sure you want to change the base?
Conversation
a77cf77
to
c1635b1
Compare
Signed-off-by: EsipovPA <[email protected]>
c1635b1
to
dafb769
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have to say that personally, I'm not a fan. int
and float
are not the same thing, and thus I think any conversion like this should be up to the user to explicitly do. I'd like to hear opinions from @ros2/team, though.
Hello @clalancette! Thanks for the quick reply. It seems to me that this is a case of a widening conversion, that should be an option for these types of fields. In case of C++ code, the int values would be casted to float automatically, when passed to For example, instead of something like this
or
user should be able to write just
and get the float value without raised exceptions. Still confused about the failed test though. Checking |
Yeah, I understand what you are trying to do. I still don't think it is a good idea, even if C++ allows it (there is nothing we can do about that, it is part of the language). |
At the minimum, the asserts should be converted to Python exceptions. I set one field of a
with no stacktrace or what the actual type provided was. This took around 20 minutes to debug with gdb and some manual type checking. I do strongly believe that ints should be implicitly converted—I would argue that using |
Added casting int to floats for float-typed fields in messages
Added test for new feature
Closes this issue