-
Notifications
You must be signed in to change notification settings - Fork 95
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
Giving force mode parameters as arguments when calling startForceMode #208
base: master
Are you sure you want to change the base?
Conversation
39e858b
to
d7f1fc3
Compare
And in client library
bd4ddd9
to
535b083
Compare
Formatting Co-authored-by: Felix Exner (fexner) <[email protected]>
Formatting Co-authored-by: Felix Exner (fexner) <[email protected]>
Co-authored-by: Felix Exner (fexner) <[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.
Could you please change the exceptions to more specific types? This would be great!
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 Meant to request changes, not to approve...
Added them to the various checks for starting force mode. I created IncompatibleRobotVersion instead of using VersionMismatch, as the VersionMismatch would not give enough information in my opinion.
322eb05
to
179dd05
Compare
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 think it would be best to use the full version information in the IncompatibleRobotVersion
exception.
The proposed changes obviously require changes where the exception is raised.
Co-authored-by: Felix Exner <[email protected]>
After further thought, I dont think the Incompatible version exception is appropriate for one of the exception calls, as the robot is not actually incompatible, the function just needs more information. The new exception conveys that, i think. I can't quite make my mind up about whether it should mention that this is due to the robot version, though, so I have left it out for now.
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 think this is looking good now. Thank you @URJala
No description provided.