Repository navigation
fix(HF-268): support consecutive percent operators - #1773
Tobiadefami wants to merge 7 commits into
Conversation
Formulas such as =5%% failed to parse even though =(5%)% worked. Consume consecutive postfix percent operators and wrap the preceding expression for each token, so =5%% returns 0.0005 and =5%%% returns 0.000005, matching Excel. Preserve each operator's whitespace and the existing operator precedence. Document the behavior in the operator guide and Unreleased changelog. Validated with the matching fix/HF-268 test branch: 502 Jest suites and 6,254 tests passed, and Chrome and Firefox each passed 6,254 tests. Compilation and performance benchmarks passed. Full lint passed with zero errors and existing warnings using the test TypeScript project.
|
@Tobiadefami thanks for the pull request. No CLA step needed here — our records show you signed the Contributor License Agreement on 2026-07-31. That signature came from our previous signing form and has been carried over, so there is nothing for you to re-sign. |
|
Task linked: HF-268 Promile operator ( |
Add the pull request reference to the consecutive percent operators changelog entry, matching the surrounding entries. Validated the PR target and checked the Markdown diff for whitespace errors.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 5361c44. Configure here.
Performance comparison of head (2e0fe8f) vs base (87e2fe0) |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
hyperformula-docs | 2e0fe8f | Commit Preview URL Branch Preview URL |
Nov 03 2026, 08:50 AM |
marcin-kordas-hoc
left a comment
marcin-kordas-hoc
left a comment
There was a problem hiding this comment.
Recommendation: Approve, blocked on a companion PR.
I reproduced the reported parsing defect on the current base: =1%%, =1%%%, =-1%%, =5%%
and =5%%% all fail to parse before this change ("Redundant input, expecting EOF but found: %"),
and all five parse and evaluate correctly after it (0.0001, 0.000001, -0.0001, 0.0005 and
0.000005 respectively). Measured via MS Graph against a live Excel Online session: Excel accepts
every one of these forms, plus a chained-percent-on-a-reference case and a percent/exponent
precedence case, and the fixed engine's values match Excel's, including the precedence case
(2^1%% → 1.0000693171 in the engine and 1.00006931712038 in Excel — agreement to 10 decimal
places).
I ran the full engine test suite together with the paired hyperformula-tests branch
(fix/HF-268) locally: 502 suites, 6254 tests passing, matching this PR's own reported numbers,
and confirmed the same result from this PR's CI log for the browser (Karma) run.
One blocker before merge: fix/HF-268 on hyperformula-tests has no open PR. Since our test repo
CI resolves the paired branch by name, it did gate this PR's own green status — but with no PR
open there, those test changes have no path into hyperformula-tests's develop. I confirmed
the consequence directly: running the four touched paired spec files against hyperformula-tests
develop (instead of fix/HF-268) while keeping this fix gives 2 failing suites / 3 failing
tests, because develop still encodes the pre-fix expectation that %% doesn't parse. Merging
this PR alone would redden our own develop the next time the paired suite runs against it.
Could you open a PR for fix/HF-268 so the two land together?
One clarification on intent, since the task title can mislead: HF-268's intended semantics are
Excel's chained percent — each % divides by 100, so 5%% = 0.0005 — not a promille (‰, ÷1000)
operator. Verified against live Excel Online and matched by this PR.
|
@marcin-kordas-hoc Thanks for catching this. I opened the companion test PR (hyperformula-tests #65). It targets develop from fix/HF-268, so the tests can be reviewed and merged alongside engine #1773. |
Gather the consecutive PercentOp tokens in the MANY loop and build the nested percent AST with a single reduce afterwards, keeping token consumption separate from AST construction. Behavior is unchanged: =5%% returns 0.0005 and each operator keeps its leading whitespace. Validated with the matching fix/HF-268 test branch: 502 Jest suites and 6,254 tests passed (3 skipped). Type-check and lint passed with zero errors.
marcin-kordas-hoc
left a comment
marcin-kordas-hoc
left a comment
There was a problem hiding this comment.
Approving at 0fc1808. The blocker from my previous review is resolved: the companion tests are in hyperformula-tests#65 (branch fix/HF-268), and this PR's unit and browser jobs ran against that branch on this exact commit (502 suites / 6,254 tests in Jest, 6,254 in Chrome and in Firefox).
I re-checked the collect-then-fold rewrite of rightUnaryOpAtomicExpression against the previous version:
- Behavior is unchanged: parsing 20,000 generated formulas (chained
%with arbitrary spaces and newlines, references, ranges, arrays, functions, unary signs, malformed input) gives byte-identical ASTs and errors before and after, with and without parser-cache hits. - Fourteen one-line breakages of the new loop (dropped or reordered whitespace, wrong operand wrapped, only one or two
%applied,reduceRight, empty list, off-by-one at exactly one%,MANY->AT_LEAST_ONE) are each caught by the tests in #65. - The grammar that Chevrotain records for the rule is identical, so pushing into a local array inside
MANYdoes not interfere with its recording phase. - All 43 Excel Online cases from my earlier pass still match (40 at the default config; the three array and range cases match under
useArrayArithmetic: true, the same as for single%).
Please merge this together with hyperformula-tests#65; each one alone turns the other repository's develop red. Two small leftovers in the description, not blocking: the checklist is unticked and #65 is not linked from the body.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
1 similar comment
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
marcin-kordas-hoc
left a comment
marcin-kordas-hoc
left a comment
There was a problem hiding this comment.
blocking: the description is still the previous template with Context, How did you test your changes and the checklist left empty, and it has no Compatibility, Public API changes or Out of scope sections; the Definition of Done asks for every section of the current template to be answered. Its numbers are also out of date: it says 502 suites and 6,254 tests, while the unit job on 5f2a12c ran 520 suites and 6,763 tests against fix/HF-268. Squash merge uses the description as the commit message, so this is the text that lands on develop. A filled-in version below, with values I measured in Excel Online today.
Suggested description
### Context
Formulas with consecutive percent operators, such as `=5%%`, failed to parse ("Redundant input, expecting EOF but found: %"), while `=(5%)%` worked. Excel accepts consecutive `%` and divides by 100 for each one. The `rightUnaryOpAtomicExpression` rule now accepts zero or more `%` after an operand and nests one percent node per operator, so `=5%%` returns `0.0005`. Each operator keeps its own leading whitespace, and operator precedence does not change.
### How did you test your changes?
Paired tests: hyperformula-tests#65, branch `fix/HF-268`. They cover evaluation (precedence, references, named expressions, coercion, errors, arrays), the parsed AST, the round trip through the unparser with whitespace, and the malformed `=%%` and `=5%%2`. With the parser change reverted, 20 of them fail. CI on this commit: Jest 520 suites, 6,763 tests passed, 3 skipped; Chrome and Firefox 6,750 tests each.
### Compatibility with Excel and Google Sheets
Excel Online. Google Sheets not checked.
| Formula | Excel | Google Sheets | HyperFormula |
|---|---|---|---|
| `=5%%` | 0.0005 | not checked | 0.0005 |
| `=5%%%` | 0.000005 | not checked | 0.000005 |
| `=5%%+1` | 1.0005 | not checked | 1.0005 |
| `=2*5%%` | 0.001 | not checked | 0.001 |
| `=5%%^2` | 0.00000025 | not checked | 0.00000025 |
| `=-5%%` | -0.0005 | not checked | -0.0005 |
| `=SUM(2,3)%%` | 0.0005 | not checked | 0.0005 |
| `="5"%%` | 0.0005 | not checked | 0.0005 |
| `="oops"%%` | #VALUE! | not checked | #VALUE! |
| `=#DIV/0!%%` | #DIV/0! | not checked | #DIV/0! |
| `=SUM({5,100}%%)` with `useArrayArithmetic: true` | 0.0105 | not checked | 0.0105 |
| `=%%`, `=5%%2` | rejected as invalid | not checked | parsing error |
### Public API changes
None.
### Out of scope and follow-ups
None.Plus the Bug fix and Change to the documentation boxes under Types of changes, and the checklist.
Merge note: please merge this pull request with Squash and merge.
…angelog Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@marcin-kordas-hoc Thanks. I applied your changelog wording and replaced the description with your version, with these changes: the Excel column is credited to your measurement, the Polish round trip from hyperformula-tests#65 is added, the failing count with the parser reverted is now 21, and the test numbers are from the new head. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1773 +/- ##
===========================================
- Coverage 97.28% 97.28% -0.01%
===========================================
Files 207 207
Lines 16218 16217 -1
Branches 3588 3555 -33
===========================================
- Hits 15777 15776 -1
+ Misses 441 433 -8
- Partials 0 8 +8
🚀 New features to boost your workflow:
|
marcin-kordas-hoc
left a comment
marcin-kordas-hoc
left a comment
There was a problem hiding this comment.
Approving at 2e0fe8f: the description now follows the template, the changelog gives the new value, and the Polish round trip in hyperformula-tests#65 fails with the parser change reverted. Since #1738 merged, CHANGELOG.md conflicts with develop; keep both Fixed entries. nit: the description becomes the squash commit message, so drop the user account from the Compatibility line. Please squash-merge together with hyperformula-tests#65.
Context
Formulas with consecutive percent operators, such as
=5%%, failed to parse ("Redundant input, expecting EOF but found: %"), while=(5%)%worked. Excel accepts consecutive%and divides by 100 for each one. TherightUnaryOpAtomicExpressionrule now accepts zero or more%after an operand and nests one percent node per operator, so=5%%returns0.0005. Each operator keeps its own leading whitespace, and operator precedence does not change.How did you test your changes?
Paired tests: hyperformula-tests#65, branch
fix/HF-268. They cover evaluation (precedence, references, named expressions, coercion, errors, arrays), the parsed AST, the round trip through the unparser with whitespace and in Polish, and the malformed=%%and=5%%2. With the parser change reverted, 21 of them fail. CI on this commit: Jest 520 suites, 6,764 tests passed, 3 skipped; Chrome and Firefox 6,751 tests each.Compatibility with Excel and Google Sheets
Excel values measured in Microsoft Excel Online by @marcin-kordas-hoc. Google Sheets not checked.
=5%%=5%%%=5%%+1=2*5%%=5%%^2=-5%%=SUM(2,3)%%="5"%%="oops"%%=#DIV/0!%%=SUM({5,100}%%)withuseArrayArithmetic: true=%%,=5%%2Public API changes
None.
Out of scope and follow-ups
None.
Types of changes
Related issues
Checklist
dev-docs/pages this change affects – or none needed it.