-
Notifications
You must be signed in to change notification settings - Fork 19
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
[WIP] RobotCollisionChecker refactor #342
Conversation
self.collision_saver = CollisionOptionsStateSaver( | ||
self.checker, collision_options) | ||
|
||
def __enter__(self): |
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.
Are these duplicated __enter__
and __exit__
methods intentional?
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.
Fixed. Good catch. 👍
Except the one issue above, I like this change. I have three comments:
|
Ok, I thought about this a bit, and this seems like the most reasonable solution (although the naming issue in my first bullet is still a bit confusing). |
I agree that the naming is a mess. It was already confusing and introducing an extra level of factories makes it more so. Any suggestions about how we can rename the factory and the context manager to make this more clear?
This would require that we create a
Yes. In the future, we might want to do more here. For example, we could configure how the baking is done. |
No description provided.