fix(vars): match Python round semantics#6769
Conversation
Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com>
Merging this PR will not alter performance
Comparing 
Footnotes
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d4de756510
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
|
|
||
| const sign = value < 0 || Object.is(value, -0) ? "-" : ""; | ||
| const result = Number(`${sign}${rounded}e${-ndigits}`); |
There was a problem hiding this comment.
When rounding very large finite values with negative precisions near -308, this conversion silently overflows the exact BigInt result to Infinity. For example, pyRound(Number.MAX_VALUE, -308) returns Infinity, while Python's round(1.7976931348623157e308, -308) raises OverflowError instead of producing a non-finite value. This means compiled frontend expressions can introduce Infinity for finite inputs in cases where the helper is intended to mirror Python semantics; please guard the conversion and handle overflow consistently.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR replaces
Confidence Score: 5/5Safe to merge — the change is well-scoped, the algorithm is arithmetically correct, and the 15 end-to-end test cases cover all boundary conditions. The BigInt-based round-half-even algorithm produces correct results for the full set of test cases. The Python-side type plumbing (int for ndigits==0, float otherwise) is preserved. No pre-existing code paths are broken beyond the intentional change in rounding output. No files require special attention. Important Files Changed
Reviews (2): Last reviewed commit: "fix(vars): match Python round semantics" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| export default function pyRound(value, ndigits) { | ||
| if (!Number.isFinite(value) || ndigits > 323) return value; | ||
| if (ndigits < -308) return value * 0; |
There was a problem hiding this comment.
ndigits values
BigInt() throws a RangeError for non-integer floats (e.g. BigInt(2.5) → RangeError). Because shift = binaryExponent + ndigits (line 24) uses ndigits directly, any fractional ndigits — such as a Reflex state variable typed as float with a non-integer runtime value like 2.5 — would cause BigInt(shift) to throw and crash the frontend render. Adding if (!Number.isInteger(ndigits)) return value; after the existing early-return guards would make the function robust against this class of misuse.
| (-1.2345, -309, "-0"), | ||
| (float("inf"), 2, "Infinity"), | ||
| ], | ||
| ) | ||
| def test_number_round_python_semantics(value, ndigits, expected): | ||
| rounded = round(LiteralNumberVar.create(value), ndigits) | ||
| var_data = rounded._get_all_var_data() | ||
| assert var_data is not None | ||
| assert any( | ||
| imported.tag == "pyRound" and imported.is_default | ||
| for imported in dict(var_data.imports)["$/utils/helpers/round.js"] | ||
| ) | ||
|
|
||
| result = subprocess.run( | ||
| [ | ||
| "node", | ||
| "--input-type=module", | ||
| "--eval", | ||
| f"{ROUND_HELPER.read_text(encoding='utf-8')}\nconsole.log({rounded!s})", | ||
| ], | ||
| check=True, |
There was a problem hiding this comment.
test_number_round_python_semantics requires Node.js in PATH
The parametrized test spawns node --input-type=module --eval ... via subprocess.run(check=True, ...). Any environment where node is not on PATH (minimal CI containers, pip install smoke tests, etc.) will cause all 15 parametrized cases to fail with a FileNotFoundError or CalledProcessError, not a proper pytest.skip. Consider guarding with pytest.importorskip or a skipif decorator based on shutil.which("node") so the suite degrades gracefully instead of erroring out.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
2 participants
All Submissions:
Type of change
Description
Compiled
NumberVarrounding previously used JavaScriptMath.roundandNumber.prototype.toFixed. These APIs differ from Python on half ties, andtoFixedrejects negativendigitsvalues.This change routes compiled rounding through a frontend helper that decomposes
IEEE 754 values into exact rational numbers and applies round-half-even. It adds
support for negative precision while preserving the existing static result types.
Testing
uv run pytest tests/units/test_var.py -q(223 passed)uv run pytest tests/units --cov --no-cov-on-fail --cov-report=(6664 passed, 17 skipped, 78.09% coverage)uv run ruff check .uv run ruff format --check .uv run pyright reflex tests(0 errors)uv run pre-commit run --files packages/reflex-base/src/reflex_base/.templates/web/utils/helpers/round.js packages/reflex-base/src/reflex_base/vars/number.py packages/reflex-base/news/6757.bugfix.md tests/units/test_var.pynode --check packages/reflex-base/src/reflex_base/.templates/web/utils/helpers/round.jsuv run towncrier check --config pyproject.toml --dir packages/reflex-base --compare-with origin/mainThe regression test executes the generated JavaScript in Node and covers positive
and negative half ties, negative precision, binary-float edge cases, extreme
precision, signed zero, non-finite values, helper imports, and result types.
Changes To Core Features: