Fix for prior fix. #34
Conversation
|
Did the previous commit just have an issue with comments? ROS seems to use the entire line after the equals sign (up to a comment if it's not a string) to calculate the md5 text - so ( I had to do something similar in the indigo branch actually - ended up consulting |
chfritz
force-pushed
the
minor_fix_for_constants
branch
2 times, most recently
from
fdd2b9e to
c595cd1
Compare
|
Thanks for the pointer. That is indeed a better way of dealing with the issue and I've adopted that now, but I still needed to store the constant value as a string, rather than a parsed value in order to stop javascript from doing things like turning 1.0 into 1. |
|
Do you still have a problem with "1.0" vs "1.00" in this change? I see you're looking for whitespace, but not trailing zeros. |
utils/messages.js
Outdated
| name : fieldName | ||
| , type : fieldType | ||
| , value : parsedConstant | ||
| <<<<<<< HEAD |
There was a problem hiding this comment.
oops, sorry. fixed now.
utils/messages.js
Outdated
| // simple value | ||
| that[field.name] = values ? values[field.name] : | ||
| (field.value || fieldsUtil.getDefaultValue(field.type)); | ||
| that[field.name] = values ? values[field.name] : null; |
There was a problem hiding this comment.
no this one was intended.
There was a problem hiding this comment.
Won't this force users to specify values for all the fields in their messages?
const header = new std_msgs.msg.Header()
pub.publish(header); // invalid
whereas before that would have worked
1.0 is different from 1.00 in ros' message md5 text so trailing zeros are correct. |
chfritz
force-pushed
the
minor_fix_for_constants
branch
2 times, most recently
from
f654905 to
fec3d80
Compare
|
I'm actually not sure what the right behavior is. We also want to allow users to leave (optional) values unspecified (null). Maybe we just need to set them explicitly to null then already in the JS object. |
|
I don't think ROS 1.0's message serialization scheme allows for optional values. If we want optional arguments when sending messages we typically will use constants or something and check against those on the other end. It does start to get cumbersome when you want optional complex fields. Automatically providing default values for fields allows devs sending a message to ignore certain fields if they don't care about them though - provided your subscriber interprets the default values correctly... |
…otics-opensource#33) Prior to this commit constant values like "1.0", i.e., floats that are integer, were not correctly treated in the md5sum calculation -- they were abbreviated to "1", causing incorrect md5sums. This commit fixes that by using the original string value for md5sum calculation (up to the start of a comment (for strings) or the first whitespace character for any other field type). - also fixing some wrong logic for null values
chfritz
force-pushed
the
minor_fix_for_constants
branch
from
fec3d80 to
44a9f81
Compare
|
I guess you are right. I was thrown off by some null values I saw in our messages, but I guess those are just used as constants the same way you guys are using them. Fixed that now, allowing for null values but providing defaults for fields that are actually |
|
Awesome - I'll release a new version with this and then pull the indigo merge changes in. |
…otics-opensource#33) (RethinkRobotics-opensource#34) Prior to this commit constant values like "1.0", i.e., floats that are integer, were not correctly treated in the md5sum calculation -- they were abbreviated to "1", causing incorrect md5sums. This commit fixes that by using the original string value for md5sum calculation (up to the start of a comment (for strings) or the first whitespace character for any other field type). - also fixing some wrong logic for null values
3 participants
Sorry, there was an issue with the previous attempt of fixing this.
This one seems to work.