Conversation
Signed-off-by: Juan Ignacio Ubeira <jubeira@ekumenlabs.com>
|
Partial CI:
Running remaining jobs now. |
|
Full CI:
|
ivanpauno
left a comment
There was a problem hiding this comment.
LGTM! I let some minor comments.
rclpy/test/test_node.py
Outdated
| ('from_value', 0.0, ParameterDescriptor(floating_point_range=[fp_range])), | ||
| ('to_value', 10.0, ParameterDescriptor(floating_point_range=[fp_range])), | ||
| ('in_range', 4.5, ParameterDescriptor(floating_point_range=[fp_range])), | ||
| ('str_value', 'I am no integer', ParameterDescriptor(floating_point_range=[fp_range])), |
There was a problem hiding this comment.
I am not integer -> I am not float
Signed-off-by: Juan Ignacio Ubeira <jubeira@ekumenlabs.com>
|
CI after comments:
|
jacobperron
left a comment
There was a problem hiding this comment.
A couple nitpicks, otherwise LGTM.
In the future, it might be nice to define an interface for constraint checking, so the node doesn't have to contain the logic for all possible constraints. Since we only have two types of constraints for now, I think it's fine to leave it as-is.
| self.assertEqual(self.node.get_parameter('in_range').value, 4.5) | ||
|
|
||
| # Change in_range parameter to int; ranges will not apply. | ||
| result = self.node.set_parameters([Parameter('in_range', value=12)]) |
There was a problem hiding this comment.
That's subtle. I hope users don't forget (or accidentally add) a decimal point in their code, otherwise they may experience unexpected behavior. I guess this is a case for type constraints.
This is just an observation; nothing to be done.
There was a problem hiding this comment.
Indeed.
Note that in this case I'm making the Parameter guess the type; using the old API where the type had to be explicitly given to the parameter you wouldn't have this problem.
In any case, I also think the solution is eventually enforcing the types (now for Dashing, or later for a patch release).
…with Windows. Signed-off-by: Juan Ignacio Ubeira <jubeira@ekumenlabs.com>
|
Thanks for the thoughtful comments @jacobperron! |
|
CI:
|
|
Failure on Windows seems to be unrelated; merging now! |
|
The Windows CI failure has appeared before and is in test_rclcpp. When |
Closes #350.
See ros2/rclcpp#728 for details and related discussion.