Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds two new helper functions any and all to the expression language, which evaluate whether any or all items in a list are true. The implementation includes:
- Parser support for the new
anyandallkeywords - Expression enum variants and evaluation logic for both functions
- Comprehensive test coverage including standalone tests and integration with existing
for mapfunctionality
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/for_map_filter.rs | Adds test cases for any, all, nested for operations, and their combinations |
| src/reval.lalrpop | Registers any and all as keywords and adds parser rules for function calls |
| src/expr/mod.rs | Defines Any and All expression variants and their Display implementations |
| src/expr/keywords.rs | Adds "any" and "all" to the reserved keywords list |
| src/expr/eval/mod.rs | Implements evaluation logic for any and all functions |
| src/expr/constructor.rs | Adds constructor methods for creating Any and All expressions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for item in vec { | ||
| match item { | ||
| Value::Bool(true) => return Ok(Value::Bool(true)), | ||
| Value::Bool(false) => continue, | ||
| _ => return Err(Error::InvalidType), |
There was a problem hiding this comment.
The any function silently ignores non-boolean values in the vector. If a vector contains non-boolean items (like integers, strings, etc.), they are treated as false which may lead to incorrect results. Consider validating that all items in the vector are booleans and returning an error for non-boolean values, similar to how other type-strict functions in the codebase (like not) handle invalid types.
| match value { | ||
| Value::Vec(vec) => { | ||
| for item in vec { | ||
| match item { | ||
| Value::Bool(true) => continue, |
There was a problem hiding this comment.
The all function silently ignores non-boolean values in the vector. If a vector contains non-boolean items, they are treated as true which may lead to incorrect results. For example, all([true, "string", true]) would return true even though "string" is not a boolean. Consider validating that all items in the vector are booleans and returning an error for non-boolean values, similar to how other type-strict functions in the codebase (like not) handle invalid types.
| async fn should_evaluate_any() { | ||
| assert_eq!( | ||
| eval_expr("any([false, false, true, false])", ()).await, | ||
| true.into() | ||
| ); | ||
| assert_eq!( | ||
| eval_expr("any([false, false, false])", ()).await, | ||
| false.into() | ||
| ); | ||
| } |
There was a problem hiding this comment.
The test coverage for the any function is missing important edge cases. Consider adding tests for: 1) an empty array (should return false), 2) an array with Value::None (to verify None handling), and 3) an array with non-boolean values (to verify type validation behavior).
| async fn should_evaluate_all() { | ||
| assert_eq!(eval_expr("all([true, true, true])", ()).await, true.into()); | ||
| assert_eq!( | ||
| eval_expr("all([true, false, true])", ()).await, | ||
| false.into() | ||
| ); | ||
| } |
There was a problem hiding this comment.
The test coverage for the all function is missing important edge cases. Consider adding tests for: 1) an empty array (should return true by convention in most languages), 2) an array with Value::None (to verify None handling), and 3) an array with non-boolean values (to verify type validation behavior).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
discosultan
left a comment
discosultan
left a comment
There was a problem hiding this comment.
LGTM
Can confirm that after this change, the original use case which started all this is finally working.
No description provided.