Provide defaults for ParameterValue fields #18
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,8 +6,8 @@ | |
| uint8 type | ||
|
|
||
| # "Variant" style storage of the parameter value. | ||
| bool bool_value False | ||
|
There was a problem hiding this comment. Please use There was a problem hiding this comment. to clarify, is your comment just for style? or is There was a problem hiding this comment. It is not supposed to work given that our specification defines There was a problem hiding this comment. ok thanks for the explanation -- just wanted to check we know why it's working as is. fixed in 43081b3 |
||
| int64 integer_value 0 | ||
| float64 double_value 0.0 | ||
| string string_value "" | ||
| byte[] bytes_value [] | ||
|
There was a problem hiding this comment. The default value for the array is unnecessary. An array is always empty by default. |
||
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.
Actually should we initialize this too?
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.
yeah good point, I guess that PARAMETER_NOT_SET would be a good default. Not sure how we refer to message constants from another msg file though.
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.
Hm, I guess we can't at the moment...
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.
My reasoning for not putting a default for this was that, while all-but-one of the
_valuefields will be unused, thetypeis not "optional" so the client library should always initialise it itselfThere 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.
Should we open a ticket for this somewhere (rosidl?) that would be a useful feature
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've got two thumbs up on my last comment so will consider this question resolved. (in the future, reactions don't send notifications (at least not to me!) so comments would be appreciated)
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.
👍 sorry forgot about that, yeah it would be great if github allowed to enable notification for "reactions"