Make sure to check errors when expanding the topic name. #58
Conversation
We need to catch ValueErrors when actually doing the expansion, then InvalidTopicNameException when doing the validation. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
ros2topic/ros2topic/verb/echo.py
Outdated
| try: | ||
| expanded_name = expand_topic_name(topic_name, node.get_name(), node.get_namespace()) | ||
| except ValueError as e: | ||
| raise RuntimeError("Topic name '%s' is invalid" % (topic_name)) |
There was a problem hiding this comment.
The docblock of the function says the following which doesn't seem to match the error message:
:raises: ValueError if the topic name, node name or namespace are invalid
There was a problem hiding this comment.
Does this RuntimeError get handled elsewhere, or does the user see the RuntimeError traceback? If the latter it looks like the ValueError raised by rclpy already includes a similar message. How about writing that message, e.args[0], to stderr instead?
There was a problem hiding this comment.
The RuntimeError is being used to signal an error where the user only needs to see the message but not the stack trace: see
ros2cli/ros2cli/ros2cli/cli.py
Lines 67 to 69 in ce77579
By handling this in a single place it allows to easily change the behavior in the future. That would not be the case if every code directly decides what to do.
There was a problem hiding this comment.
A RuntimeError gets handled by the higher layers of ros2cli and prints out a nice error message. My original implementation here did what @sloretz proposed, but the problem is that the error message it actually generates is kind of ugly:
topic name '/test_topic/' is invalid: topic name is invalid, at /home/clalancette/ros2_ws/src/ros2/rcl/rcl/src/rcl/expand_topic_name.c:67
That being said, if we think that is a better way to go, I'll change it to that.
There was a problem hiding this comment.
I agree with @dirk-thomas that the value error doesn't imply topic is bad only. Other than that I think the runtime error pattern is fine.
If you want to make the error message pretty then you can reformat it when putting it into the runtime error. Or if it's a general improvement to the error message, you could make it in rclpy's function.
There was a problem hiding this comment.
All right, I updated this to use the original exception instead. I'll do a more general cleanup to that error message in another PR.
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
clalancette
added
in review
|
Thanks, merging. |
suppress repeated SIGINTs in order to cleanly shutdown
We need to catch ValueErrors when actually doing the expansion,
then InvalidTopicNameException when doing the validation.
Signed-off-by: Chris Lalancette clalancette@openrobotics.org