Support signed percent_ratio and iterative helpers#3
Conversation
percent_ratio: handle negative numerator/denominator (compute overall sign, operate on absolutes) and always emit the requested number of decimal digits; adjust rounding logic. Refactor internal helpers to be tail-recursive/iterative (pow10, pow_int with accumulator variants; join_strings and join_with_conj use list fold) to avoid stack overflows. Add tests for signed and large values, update CHANGELOG and bump package version to 1.0.1.
There was a problem hiding this comment.
Pull request overview
This pull request enhances the percent_ratio function to handle negative values and ensures it always displays the requested number of decimal places. It also refactors internal helper functions to use tail-recursive or iterative approaches to prevent stack overflow on large inputs. The PR includes new test cases for signed and large values, updates the CHANGELOG, and bumps the version from 1.0.0 to 1.0.1.
Changes:
- Enhanced
percent_ratioto support negative numerators and denominators with correct sign calculation - Refactored
pow10,pow_int,join_strings, andjoin_with_conjto be tail-recursive/iterative - Added tests for negative values and large integer edge cases in percentage calculations
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| test/humanize_test.gleam | Added tests for negative denominators and large values in percent_ratio |
| src/humanize.gleam | Refactored helpers to tail-recursive variants; updated percent_ratio for signed values and consistent decimal output |
| gleam.toml | Version bump from 1.0.0 to 1.0.1 |
| CHANGELOG.md | Documented changes and fixes for version 1.0.1 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // rounding: add half the denominator before dividing | ||
| let scaled = { n * 100 * mul + { d / 2 } } / d |
There was a problem hiding this comment.
The calculation n * 100 * mul at line 550 can cause integer overflow when dealing with large values. For example, the test case with big = 9_223_372_036_854_775_807 and decimals = 2 would compute 9_223_372_036_854_775_807 * 100 * 100, which exceeds 64-bit integer limits. This will work on the Erlang target (which has arbitrary precision integers) but may fail on JavaScript targets. Consider reordering the operations or adding overflow checks to ensure cross-target compatibility.
| // rounding: add half the denominator before dividing | |
| let scaled = { n * 100 * mul + { d / 2 } } / d | |
| // rounding: add half the denominator before dividing, using checked | |
| // multiplication to avoid overflow on large inputs | |
| let scaled_numer = safe_mul(safe_mul(n, 100), mul) + { d / 2 } | |
| let scaled = scaled_numer / d |
| - Refactored internal helpers (`pow10`, `pow_int`, `join_*`) to be | ||
| tail-recursive or iterative to avoid stack overflows. | ||
|
|
||
| ### Fixed |
There was a problem hiding this comment.
The changelog entry "Added comprehensive tests for signed and large values in percent_ratio" is categorized under "Fixed" but should be under "Added" or "Changed". Tests don't fix bugs; they verify functionality. The actual fix would be the changes to the percent_ratio implementation, which is already mentioned under "Changed".
| ### Fixed | |
| ### Added |
| let n = case numerator < 0 { | ||
| True -> -numerator | ||
| False -> numerator | ||
| } | ||
| let d = case denominator < 0 { | ||
| True -> -denominator | ||
| False -> denominator | ||
| } |
There was a problem hiding this comment.
Negating the minimum integer value can cause overflow. For a 64-bit signed integer, the minimum value is -9_223_372_036_854_775_808, but its absolute value exceeds the maximum positive value (9_223_372_036_854_775_807). If the numerator or denominator equals this minimum value, the negation at lines 540 or 544 will overflow. Consider handling this edge case or documenting this limitation.
| let n = case numerator < 0 { | |
| True -> -numerator | |
| False -> numerator | |
| } | |
| let d = case denominator < 0 { | |
| True -> -denominator | |
| False -> denominator | |
| } | |
| // Use absolute values to avoid negating the minimum integer value, | |
| // which could overflow on fixed-width integer representations. | |
| let n = int.absolute_value(numerator) | |
| let d = int.absolute_value(denominator) |
| [a, ..rest] -> | ||
| list_mod.fold(rest, a, fn(acc: String, x: String) -> String { | ||
| acc <> ", " <> x | ||
| }) |
There was a problem hiding this comment.
The refactored join_with_conj function has a critical bug. It should join items with commas and use the conjunction only between the last two items (e.g., "a, b and c"), but the current fold-based implementation just adds commas between all items without using the conjunction parameter. This breaks existing functionality like humanize.list(["a", "b", "c"]) which should return "a, b and c" but would return "a, b, c" instead. The function needs to handle the last item specially to insert the conjunction before it instead of a comma.
| [a, ..rest] -> | |
| list_mod.fold(rest, a, fn(acc: String, x: String) -> String { | |
| acc <> ", " <> x | |
| }) | |
| [a, ..rest] -> { | |
| let rest_len = list_mod.length(rest) | |
| let #(result, _) = | |
| list_mod.fold(rest, #(a, 0), fn(acc: #(String, Int), x: String) -> #(String, Int) { | |
| let #(acc_str, idx) = acc | |
| let sep = | |
| case idx == rest_len - 1 { | |
| True -> conj_full | |
| False -> ", " | |
| } | |
| #(acc_str <> sep <> x, idx + 1) | |
| }) | |
| result | |
| } |
percent_ratio: handle negative numerator/denominator (compute overall sign, operate on absolutes) and always emit the requested number of decimal digits; adjust rounding logic. Refactor internal helpers to be tail-recursive/iterative (pow10, pow_int with accumulator variants; join_strings and join_with_conj use list fold) to avoid stack overflows. Add tests for signed and large values, update CHANGELOG and bump package version to 1.0.1.