-
Notifications
You must be signed in to change notification settings - Fork 221
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove agnostic lib #82
Conversation
rclpy/CMakeLists.txt
Outdated
@@ -57,6 +57,7 @@ if(WIN32) | |||
endif() | |||
|
|||
target_link_libraries(${PROJECT_NAME} | |||
${rcl_LIBRARIES} |
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.
Isn't this redundant with the ament_target_dependencies
call below?
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.
Absolutely, I moved it from the other library target and noticed the duplicate. Removed in a699313.
rclpy/rclpy/node.py
Outdated
# ensure that the passed node contains a valid capsule | ||
class_ = self.handle.__class__ | ||
if not class_ or class_.__name__ != 'PyCapsule': | ||
raise ValueError() |
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.
Shouldn't this have a message?
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.
It was only used in one bizarre test (which modified the internal handle of a node instance) so I thought it isn't a real world case. Added a message in 117eac9.
@@ -20,7 +20,7 @@ | |||
def run_catch_report_raise(func, *args, **kwargs): | |||
try: | |||
return func(*args, **kwargs) | |||
except: | |||
except Exception: |
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.
Is this necessary?
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.
Not necessary but recommended in PEP 8 and some linters will warn about it:
When catching exceptions, mention specific exceptions whenever possible instead of using a bare
except:
117eac9
to
4ebe9dc
Compare
Unfortunately I didn't catch this before, but the removal of the proxy object machinery has caused a regression in the check that
Please restore the proxy object stuff or check that This is the code that checked on each access of https://github.com/ros2/rclpy/pull/82/files#diff-7486716035fb1f918746b7f5b1b31b35L49 |
I created #84 to track the issue since this cannot be reopened. |
Also, after this pr, |
Since the
rclpy
lib is now agnostic to the RMW impl. there is no need to keep some functions in a separate library. And since the loaded Python extension is always the same I removed all the fancy loading logic around it.The second commit addresses a test failure. Instead of passing a known invalid parameter to the C function raise a
ValueError
instead.rclpytestst
(http://ci.ros2.org/view/nightly/job/nightly_win_deb/lastCompletedBuild/testReport/(root)/projectroot/rclpytests/).