-
Notifications
You must be signed in to change notification settings - Fork 76
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 constructors arguments deprecated since Foxy #190
Remove constructors arguments deprecated since Foxy #190
Conversation
…deprecated alternatives Signed-off-by: Ivan Santiago Paunovic <[email protected]>
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
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.
Based on CI results, it looks like some tests need to be updated 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.
LGTM pending green CI
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
This change regressed the performance tests with a cryptic exception: http://build.ros2.org/view/Rci/job/Rci__nightly-performance_ubuntu_focal_amd64/112/
After modifying the code, I was able to extract a traceback:
EDIT: Looks like the regression is because this change dropped the |
@cottsay |
There were, but it didn't make the build go yellow and they were lost in the wall of text coming out of |
I think this is an instance where ros2/ci#509 would have helped. It's on the long-term TODO list, but we need to spend some time on it. Separately, I will say that that error message is hard for the end user to tell what happened. @ivanpauno @jacobperron Could we throw a nicer exception for unknown keyword arguments? |
The error message wasn't obvious because of a mistake in this patch. |
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 found another bug.
Cleaning up things deprecated before Foxy release.
Follow up of #189.