ros-planning
Remove MoveIt exceptions
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.
Yes to getting rid of boost::noncopyable (hell, that was in there? :confused: ) and also bye bye createRobotModel (redundant method).
I would actually propose to just remove the PlanningScene(urdf_model, srdf_model) constructor right away instead of deprecating it. There is not much use in constructing a PlanningScene that way, because a RobotModel holds the same information and is the much more appropriate datastructure here.
Removing this constructor already suffices to get rid of the exceptions though and I don't think it's useful to move to a monadic factory method.
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
planning_scene::PlanningScene scene{ robot_model }
or even
auto scene{ planning_scene::PlanningScene{ robot_model } }
reads a lot better than
auto scene{ planning_scene::PlanningScene::create(robot_model).value() }
I do think
planning_scene::PlanningScene scene{ robot_model }or even
auto scene{ planning_scene::PlanningScene{ robot_model } }reads a lot better than
auto scene{ planning_scene::PlanningScene::create(robot_model).value() }
Agree with this! I'm not a fan of "always auto"
I would actually propose to just remove the PlanningScene(urdf_model, srdf_model) constructor right away instead of deprecating it.
Sure thing, will do!
The point of the create() function is that it's allowed to fail gracefully. With the constructor, we have no other choice but throwing an exception or flagging the instance invalid. I agree, however, that a plain constructor would be nice.
The point of the
create()function is that it's allowed to fail gracefully. With the constructor, we have no other choice but throwing an exception or flagging the instance invalid.
I'm aware of that, but there are simply no things left in the constructor now that need graceful failure handling. 🙂
and adding the additional .value() everywhere is only worth the hassle when there are failure cases.
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.
The point of the
create()function is that it's allowed to fail gracefully. With the constructor, we have no other choice but throwing an exception or flagging the instance invalid.I'm aware of that, but there are simply no things left in the constructor now that need graceful failure handling. slightly_smiling_face and adding the additional
.value()everywhere is only worth the hassle when there are failure cases.
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.
![mergify[bot] avatar](https://avatars.githubusercontent.com/in/10562?v=4 "Published by a pub.dev verified publisher"){kind=small}
Sadly this work seems to be abandoned. I'll open an issue to track continuing this work.
