-
Notifications
You must be signed in to change notification settings - Fork 64
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
Fix Compose Tasks #159
Fix Compose Tasks #159
Conversation
Signed-off-by: Yadunund <yadunund@openrobotics.org>
Signed-off-by: Yadunund <yadunund@openrobotics.org>
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.
Great fix, but I do have one quick request.
if (on_cancel_json.is_array()) | ||
std::vector<rmf_task_sequence::Phase::ConstDescriptionPtr> cancel = {}; | ||
const auto cancel_it = phase_json.find("on_cancel"); | ||
if (cancel_it != phase_json.end() && cancel_it.value().is_array()) |
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.
We should not silently ignore on_cancel
if it doesn't map to an array.
We can skip adding the cancellation sequence, but let's at least add a message to the errors
vector so that the operators can be informed of the issue.
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.
Oh wait, I just remembered that the schema of on_cancel
requires it to be an array. If the validator approved of the message, then it should certainly be an array. Nothing needs to be changed here.
...Although it might not hurt to issue some kind of error anyway, just in case the schema has been changed while the assumptions of this code fail to be updated. |
Signed-off-by: Yadunund <yadunund@openrobotics.org>
Add phase descriptions to builder in task deserializer.