-
Notifications
You must be signed in to change notification settings - Fork 90
feat(stdlib): add fold function #1192
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
base: main
Are you sure you want to change the base?
Conversation
Hi @jakedipity, thank you for this contribution! I ran the CI checks and there are some failing tests. For fast local iterations you can use:
|
Also, please add a changelog. Feel free to ping me here when it's ready for another review. |
fc6123e
to
748918e
Compare
@pront I've updated the PR to pass all tests and added a changelog fragment! |
Thank you @jakedipity, I will take a look on Monday 👍 |
Adds a new fold function to reduce objects and arrays into a single value based on a closure and initial value. Signed-off-by: Jacob Hull <[email protected]>
Thanks @pront, I've rebased to latest on main. |
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 PR contains a lot of changes to the closure internals, it would great to have more VRL tests like this one: https://github.com/vectordotdev/vrl/blob/main/lib/tests/tests/expressions/function_call/closure_scope_inheritance.vrl
Note, we are not limited to testing macros, we can write tests like so: https://github.com/vectordotdev/vrl/blob/main/src/stdlib/shannon_entropy.rs#L288
@@ -309,17 +309,41 @@ impl<'a> Builder<'a> { | |||
// | |||
// We set "bar" (index 0) to return bytes, and "baz" (index 1) to return an | |||
// integer. | |||
// | |||
// If one of the arguments is dependant on a closure, the parameters initial |
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.
Typo
// If one of the arguments is dependant on a closure, the parameters initial | |
// If one of the arguments is dependent on a closure, the parameters initial |
// A closure variable kind is not possible here but we need to | ||
// satisfy all variants with a match arm. | ||
VariableKind::Closure(_) => { | ||
panic!("got unexpected variable kind") |
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.
Return Err
instead.
Summary
Adds a new fold function to reduce objects and arrays into a single value based on a closure and initial value.
Change Type
Is this a breaking change?
How did you test this PR?
Test examples in the fold function definition, existing function testing macros don't support functions with closures.
Does this PR include user facing changes?
our guidelines.
Checklist
run
dd-rust-license-tool write
and commit the changes. More details here.References
closes: #1139