Add halt for flow, decorator and sync action nodes. #69
Conversation
samouwow
changed the title
|
Great work! Thanks for the PR. I will take a look on friday! |
src/runtime/forester/flow.rs
Outdated
| let running_child_cursor = tick_args | ||
| .find(RUNNING_CHILD.to_string()) | ||
| .and_then(RtValue::as_int) | ||
| .and_then(|v| Some(v as usize)); |
There was a problem hiding this comment.
.and_then(|v| Some(v as usize)) better to replace to .map(|v| v as usize)
There was a problem hiding this comment.
That makes sense, fixed.
src/runtime/forester/flow.rs
Outdated
| @@ -299,6 +365,10 @@ pub fn monitor( | |||
| pub enum FlowDecision { | |||
| PopNode(RNodeState), | |||
| Stay(RNodeState), | |||
| Halt { | |||
| new_state: RNodeState, | |||
There was a problem hiding this comment.
since it is an internal class that i used here I would suggest to
type HaltingChildCursor = i64;
Halt(RNodeState,HaltingChildCursor), or simply
Halt(RNodeState,i64),
There was a problem hiding this comment.
Fair enough, that's reasonable.
I really like the self-documenting nature of explicitly labelling the second element as the cursor for the halting child, so I'm going to go for your first suggest. I'm worried just i64 will be harder to understand and maintain going forwards.
The code has been updated.
| match flow::monitor(tpe, args.clone(), tick_args, &mut ctx)? { | ||
| match flow::monitor( | ||
| tpe, | ||
| init_args.clone(), |
There was a problem hiding this comment.
can i suggest returning the formatting?
match flow::monitor(tpe, init_args.clone(), tick_args, &mut ctx)? {
There was a problem hiding this comment.
That's just how cargo fmt / rust-analyzer auto-formats it. Does your cargo fmt format it differently?
There was a problem hiding this comment.
It did not break it into several lines but apparently, I was not using cargo fmt but rather default ide (rustrover) formater. Yeah, keep it as is then
src/runtime/forester.rs
Outdated
| @@ -225,17 +230,26 @@ impl Forester { | |||
| debug!(target:"flow[run]", "tick:{}, {tpe}. stay with the new state: {}",ctx.curr_ts(),&ns); | |||
| ctx.new_state(id, ns)?; | |||
| } | |||
| FlowDecision::Halt { .. } => { | |||
| unreachable!("A child returning running should not trigger a flow node halt decision."); | |||
There was a problem hiding this comment.
maybe better to return an error immediately here?
return Err(RuntimeError::Unexpected("A child returning running should not trigger a flow node halt decision."))
There was a problem hiding this comment.
Good suggestion, fixed.
src/runtime/forester.rs
Outdated
| // Halting must be an atomic process, it is never spread over multiple ticks so should never be seen in this flow. | ||
| RNodeState::Halting(_) => { | ||
| unreachable!( | ||
| "A flow node child should never return a state of Halting." |
There was a problem hiding this comment.
maybe better to return an error immediately here?
return Err(RuntimeError::Unexpected("A child returning running should not trigger a flow node halt decision."))
There was a problem hiding this comment.
good suggestion, fixed.
| // Halting must be an atomic process, it is never spread over multiple ticks so should never be seen in this flow. | ||
| RNodeState::Halting(_) => { | ||
| unreachable!( | ||
| "A decorator node child should never return a state of Halting." |
There was a problem hiding this comment.
and here - return instead of panic?
There was a problem hiding this comment.
Good suggestion, fixed.
|
Thank you for the great PR. The idea is very nice. |
|
Thanks for the quick review. I've implemented all your suggestions except the formatting one, since I'd rather just keep letting |
This PR modifies the
r_sequenceandr_fallbackflow nodes to call "halt" on any running children if an earlier child returns false or true, respectively.It does not implement halting for parallel nodes or async/remote action nodes.
This PR is part of #65