Raise custom error when node name is not found #413
Conversation
rclpy/rclpy/__init__.py
Outdated
| return rclpy_implementation.rclpy_init(args if args is not None else sys.argv, context.handle) | ||
| global NodeNameNonExistentError | ||
| rclpy_implementation.rclpy_init(args if args is not None else sys.argv, context.handle) | ||
| NodeNameNonExistentError = rclpy_implementation.NodeNameNonExistentError |
There was a problem hiding this comment.
Why should this be the exposed C symbol? Wouldn't an exception defined in Python make more sense here?
There was a problem hiding this comment.
I'll weight in here, because I had a similar dilemma while figuring out how to raise appropriate exceptions upon failed argument parsing in rclpy: either defining an exception on Python and importing it from C or defining it in C and importing it on Python. And the latter seems more natural to me too.
But then this. Why is that C extensions cannot be imported at module level?
There was a problem hiding this comment.
A little of history, see: #147
I would rather not limit C extensions to be loaded after the module. Instead, it should be possible to import them at module level too.
I have a solution in mind, that makes possible to import C extensions at module level, and shows a clearer relative path when testing. I will try if it works or not.
There was a problem hiding this comment.
My current solution has a big problem, the exception is only valid after rclpy_init. I will switch to an exception defined in Python if I don't successfully avoid the limitation of importing C extensions at module level.
…'t find a node name Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
ivanpauno
force-pushed
the
ivanpauno/add-error-for-non-existent-node
branch
from
8274aa1
to
0a4fe6f
Compare
Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
| @@ -83,6 +84,10 @@ | |||
| SrvTypeRequest = TypeVar('SrvTypeRequest') | |||
| SrvTypeResponse = TypeVar('SrvTypeResponse') | |||
|
|
|||
| # Re-export exception defined in _rclpy C extension. | |||
| # `Node.get_*_names_and_types_by_node` methods may raise this error. | |||
| NodeNameNonExistentError = _rclpy.NodeNameNonExistentError | |||
There was a problem hiding this comment.
@ivanpauno there was a previous comment from @dirk-thomas about not exporting to Python exceptions defined in C but the other way around. Since modules like rclpy.qos already import C extensions at the module level, this does not worsen the situation in any way.
So I'd say that we merge this as-is, to solve ros2/build_farmer#223, and then we work towards normalizing C extension use in rclpy -- whatever that means. There's a PR, #422, for it already. @dirk-thomas do you agree?
|
CI:
|
|
I forgot to push a commit in ros2/rcl#492, again CI:
|
|
I'm merging this one, we can continue the discussion on how importing C extensions in #422. |
delete-merged-branch
bot
deleted the
ivanpauno/add-error-for-non-existent-node
branch
Addresses ros2/ros2cli#322 (comment)