Conversation
|
The new Tucana version is still missing to close this => then it would build |
Signed-off-by: Raphael Götz <52959657+raphael-goetz@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds integer support to the runtime’s numeric Value representation (via tucana::shared::NumberValue), and updates standard library functions/tests/flows to operate correctly with integer and float variants. This enables implementing number helpers like std::number::remove_digits and std::number::has_digits.
Changes:
- Introduce
crates/core/src/value.rshelpers for constructing/converting numeric values (int/float) and formatting them. - Update runtime stdlib functions (number/text/array/object/boolean/http) and tests to use the new numeric representation.
- Update test flow JSON and bump
tucanadependency to a version that supports integer numbers.
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/tests/flows/03_for_each.json | Updates flow literals to the new integer number JSON shape. |
| crates/core/src/value.rs | Adds centralized helpers for NumberValue creation/conversion/formatting. |
| crates/core/src/runtime/functions/text.rs | Adjusts numeric handling for indices/byte outputs to support integers. |
| crates/core/src/runtime/functions/object.rs | Returns sizes as integer NumberValues; updates tests accordingly. |
| crates/core/src/runtime/functions/number.rs | Switches arithmetic/comparison functions to NumberValue and adds has_digits/remove_digits. |
| crates/core/src/runtime/functions/http.rs | Stores HTTP status code as integer NumberValue. |
| crates/core/src/runtime/functions/boolean.rs | Returns boolean-as-number as integer NumberValue; updates tests. |
| crates/core/src/runtime/functions/array.rs | Updates formatting and numeric aggregation (min/max/sum) for int/float NumberValues. |
| crates/core/src/lib.rs | Exposes the new value module. |
| crates/core/src/context/executor.rs | Updates value preview formatting for the new NumberValue representation. |
| crates/core/src/context/argument.rs | Adds TryFromArgument for NumberValue and updates f64 extraction via conversion. |
| Cargo.toml / Cargo.lock | Bumps tucana dependency to support integer numbers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ("std::number::has_digits", HandlerFn::eager(has_digits, 2)), | ||
| ( | ||
| "std::number::remove_digits", | ||
| HandlerFn::eager(remove_digits, 2), |
There was a problem hiding this comment.
std::number::remove_digits is registered as taking 2 args, but remove_digits only extracts one NumberValue via args!, so calls through the executor will always fail the arity check. Register it with arity 1.
| HandlerFn::eager(remove_digits, 2), | |
| HandlerFn::eager(remove_digits, 1), |
| return Signal::Failure(RuntimeError::simple_str( | ||
| "InvlaidArgumentExeption", | ||
| "Had NumberValue but no inner number value (was null)", | ||
| )); |
There was a problem hiding this comment.
The error kind string InvlaidArgumentExeption is misspelled and inconsistent with the rest of the runtime (most functions use InvalidArgumentRuntimeError). This makes error handling brittle for callers relying on error kind matching; fix the typo and align the kind string with the project’s standard.
| args!(args => value: String, index: tucana::shared::NumberValue); | ||
| let index = match number_to_i64_lossy(&index) { | ||
| Some(v) => v, | ||
| None => return arg_err("Expected a number index"), | ||
| }; | ||
|
|
There was a problem hiding this comment.
number_to_i64_lossy truncates floats toward zero, so a negative fractional index like -0.5 becomes 0 and bypasses the index < 0 validation. Validate sign using the original f64 value (or explicitly reject non-integer/negative floats) before converting to i64/usize.
| args!(args => value: String, from: tucana::shared::NumberValue, to: tucana::shared::NumberValue); | ||
| let from = match number_to_i64_lossy(&from) { | ||
| Some(v) => v, | ||
| None => return arg_err("Expected number 'from'"), | ||
| }; | ||
| let to = match number_to_i64_lossy(&to) { | ||
| Some(v) => v, | ||
| None => return arg_err("Expected number 'to'"), | ||
| }; |
There was a problem hiding this comment.
from/to are converted with number_to_i64_lossy before checking for negativity, so negative fractional inputs (e.g. -0.5) truncate to 0 and pass the from < 0 || to < 0 guard. Perform the non-negative validation on the original f64 values (or reject non-integer indices) before converting.
| ("std::number::cosh", HandlerFn::eager(cosh, 1)), | ||
| ("std::number::clamp", HandlerFn::eager(clamp, 3)), | ||
| ("std::number::is_equal", HandlerFn::eager(is_equal, 2)), | ||
| ("std::number::has_digits", HandlerFn::eager(has_digits, 2)), |
There was a problem hiding this comment.
std::number::has_digits is registered as taking 2 args, but has_digits only extracts one NumberValue via args!, so calls through the executor will always fail the arity check. Register it with arity 1.
| ("std::number::has_digits", HandlerFn::eager(has_digits, 2)), | |
| ("std::number::has_digits", HandlerFn::eager(has_digits, 1)), |
| return Signal::Failure(RuntimeError::simple_str( | ||
| "InvlaidArgumentExeption", | ||
| "Had NumberValue but no inner number value (was null)", |
There was a problem hiding this comment.
remove_digits returns the same "...was null" message whenever number_to_i64_lossy returns None, but that can also happen for non-finite floats (NaN/∞), not only when the inner number is missing. Update the error message (and/or error kind) to reflect the actual failure reason so debugging is accurate.
| return Signal::Failure(RuntimeError::simple_str( | |
| "InvlaidArgumentExeption", | |
| "Had NumberValue but no inner number value (was null)", | |
| let message = match value.number { | |
| None => "Had NumberValue but no inner number value (was null)", | |
| Some(_) => { | |
| "Could not convert NumberValue to integer (non-finite or out of range)" | |
| } | |
| }; | |
| return Signal::Failure(RuntimeError::simple_str( | |
| "InvlaidArgumentExeption", | |
| message, |
| args!(args => value: String, position: tucana::shared::NumberValue, text: String); | ||
| let position = match number_to_i64_lossy(&position) { | ||
| Some(v) => v, | ||
| None => return arg_err("Expected a number position"), | ||
| }; | ||
|
|
There was a problem hiding this comment.
Same issue as at: converting position via number_to_i64_lossy before the non-negative check allows values like -0.5 to truncate to 0 and pass validation. Validate the f64 value is non-negative before integer conversion (or reject non-integer positions).
| fn format_value_json(value: &Value) -> String { | ||
| match value.kind.as_ref() { | ||
| Some(Kind::NumberValue(v)) => v.to_string(), | ||
| Some(Kind::NumberValue(v)) => { | ||
| match v.number { | ||
| Some(kind) => { |
There was a problem hiding this comment.
format_value_json re-implements NumberValue formatting with nested matches, while other parts of the codebase now use crate::value::number_to_string. Consider using the shared helper here too to keep formatting consistent and reduce duplication.
| Signal::Success(Value { | ||
| kind: Some(Kind::NumberValue(n)), | ||
| }) => n, | ||
| }) => number_to_f64(&n).unwrap_or_default(), |
There was a problem hiding this comment.
expect_num uses unwrap_or_default(), which will silently treat a null/invalid NumberValue as 0.0 and can let tests pass when they should fail. Prefer asserting the conversion succeeds (e.g., unwrap/expect) so tests catch regressions in number handling.
| }) => number_to_f64(&n).unwrap_or_default(), | |
| }) => number_to_f64(&n).expect("Expected valid f64 NumberValue in expect_num"), |
Resolves: #118, #119, #120