-
Notifications
You must be signed in to change notification settings - Fork 3
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
Implement vars, choose/when, and expressions #32
base: main
Are you sure you want to change the base?
Conversation
…lds Values, rather than Strings.
…to the EvalContext to differentiate them.
…l needs proper revamp.
…tadata variables.
…ring a Value::Error type.
…InterpolatedContent.
… between lexer and parser clearer anyway.
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.
Really pleased with this, thanks so much both for your hard work and for your patience while I get it reviewed. Just a few areas to address in regards to error handling but I'm happy for this to be released once the println
s are gone.
fn do_parse<'a, R>( | ||
reader: &mut Reader<R>, | ||
callback: &mut dyn FnMut(Event<'a>) -> Result<()>, | ||
task: &mut Vec<Event<'a>>, | ||
depth: &mut usize, | ||
use_queue: bool, | ||
try_depth: &mut usize, | ||
choose_depth: &mut usize, | ||
current_arm: &mut Option<TryTagArms>, | ||
tag: &TagNames, | ||
content_type: &ContentType, | ||
) -> Result<()> | ||
where |
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.
This function signature is getting pretty complex but we can leave it for now as it's not part of the public API surface and there is a parser rework in progress.
.attributes() | ||
.flatten() | ||
.find(|attr| attr.key.into_inner() == b"name") | ||
{ | ||
Some(attr) => String::from_utf8(attr.value.to_vec()).unwrap(), | ||
None => { | ||
return Err(ExecutionError::MissingRequiredParameter( | ||
String::from_utf8(elem.name().into_inner().to_vec()).unwrap(), | ||
"name".to_string(), | ||
)); | ||
} | ||
}; |
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.
This parsing pattern appears a few times in this file but, again, we can leave that for the parser rework.
@@ -504,7 +517,97 @@ fn event_receiver( | |||
except_task, | |||
}); | |||
} | |||
Event::XML(event) => { | |||
Event::ESI(Tag::Assign { name, value }) => { | |||
// TODO: the 'name' here might have a subfield, we need to parse it |
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.
Is this something that needs to be addressed before release?
} | ||
} | ||
Event::ESI(Tag::When { .. }) => { | ||
println!("Shouldn't be possible to get a When tag here"); |
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.
Should this throw an error?
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.
If not we should at least avoid println
and use the logging macros.
break; | ||
} | ||
} else { | ||
println!("Somehow got something other than a When in a Choose: {when:?}",); |
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.
Error?
} | ||
|
||
#[cfg(test)] | ||
mod tests { |
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.
The tests are a large part of this file, I wonder if it's worth extracting them into the tests
directory.
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.
Not a blocker
) -> Option<Value> { | ||
evaluate_interpolated(cur, ctx) | ||
.map_err(|e| { | ||
println!("Error while evaluating interpolated expression: {e}"); |
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.
Should this throw an error?
Avoid println
matches
operatormatches_i
matchname
parameterCleanup tasks
$fn($(var), ...)
is valid in raw text ESI contextstodo!()
calls