-
Notifications
You must be signed in to change notification settings - Fork 523
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 MoveIt exceptions #1117
Remove MoveIt exceptions #1117
Conversation
8b6f89c
to
5ee9718
Compare
Yes to getting rid of I would actually propose to just remove the In theory I agree with the idea of monadic returns. I also agree with the idea of factories when constructors would need to "do some work". But in this case there is not much going on in the constructor and I do think
or even
reads a lot better than
|
Agree with this! I'm not a fan of "always auto" |
Sure thing, will do! The point of the |
I'm aware of that, but there are simply no things left in the constructor now that need graceful failure handling. 🙂 If you want to propose consistent factory methods for all classes as a standard interface, that's different (and a lot of rewriting). but then we should check how much that would blow up the caller side using moveit interfaces. |
RobotModel could be null |
RobotModel could be null
And you are telling me because the user can pass a nullptr to the constructor I need to call `.value()` on each method call?
How would you handle the case where your create method returns no value? That error condition gives you nothing you can handle logically without knowing where the model came from. Why should I consider that case in my code if I can also consider a case right before the call for my input parameter to be null? This should probably be checked already wherever you construct your robot model.
If we want to check for nullptr parameter in the constructor (which still makes sense) I would recommend `assert`, or possibly an assert that is active even with `-DNDEBUG`.
|
This pull request is in conflict. Could you fix it @henningkayser? |
1 similar comment
This pull request is in conflict. Could you fix it @henningkayser? |
This pull request is in conflict. Could you fix it @henningkayser? |
1 similar comment
This pull request is in conflict. Could you fix it @henningkayser? |
Sadly this work seems to be abandoned. I'll open an issue to track continuing this work. |
This is as part of #1038. I think using something like std::optional or std::expected is much nicer than throwing exceptions, especially with constructors. There are still some other core classes that use exceptions, with this I wanted to continue the discussion about API changes like this.