Skip to content

Commit 4aade58

Browse files
AidosAidos
authored andcommitted
fix(excel): precedence-aware parentheses + repr UX polish
- Excel renderer now wraps BinOp operands in parens when child precedence is lower than parent's, or same-precedence on the right of a non-commutative parent. ``a * (1 - b)`` emits as ``=B1 * (1 - B2)`` instead of the wrong ``=B1 * 1 - B2``; ``(a/b) ** (1/c)`` emits as ``=(B1 / B2) ^ (1 / B3)``. Pinned by the formerly-xfail tests in ``test_excel_golden.py`` plus the same-precedence counter-example. - HTML repr toolbar buttons (Formulas / Stacked) keep stable labels; on/off is signalled by the cream-tint + teal active style, animated via the existing ``120ms ease`` transition on ``label.mo-btn``. - Per-panel titles stay in the markup so the ``Stacked`` toggle reveals each sheet's name; CSS hides them in tabbed mode. - Trace-precedents stays a pure peek — focusing a formula highlights precedent cells (including out-of-tab ones via the ``mo-dep`` class) but never auto-switches tabs. Manual tab switch reveals the highlights on hidden panels.
1 parent 267ece4 commit 4aade58

4 files changed

Lines changed: 110 additions & 21 deletions

File tree

src/modeleon/compile/excel/renderer.py

Lines changed: 73 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,23 @@
6464
}
6565

6666

67+
# Operator precedence in Excel formulas. Higher binds tighter. A child BinOp
68+
# whose precedence is *strictly less* than the parent's must be wrapped in
69+
# parens; same-precedence children only need wrapping on the right side of
70+
# non-commutative operators (``a - (b - c)`` ≠ ``(a - b) - c``).
71+
_BINOP_PRECEDENCE: Dict[str, int] = {
72+
"**": 4, "^": 4,
73+
"*": 3, "/": 3, "//": 3, "%": 3,
74+
"+": 2, "-": 2,
75+
"&": 1,
76+
"==": 0, "!=": 0, "<": 0, "<=": 0, ">": 0, ">=": 0,
77+
}
78+
79+
# Non-commutative-on-the-right ops: same-precedence right child needs parens.
80+
# `a - b - c` is fine (left-associative) but `a - (b - c)` must keep them.
81+
_NON_COMMUTATIVE_RIGHT: set[str] = {"-", "/", "//", "%", "**", "^"}
82+
83+
6784
# Functions that accept an Excel range for any list-valued VarRef argument.
6885
# The translator renders those args as ranges (``B2:F2``) and leaves
6986
# scalar VarRefs / literals as normal. Covers classic aggregates (SUM,
@@ -216,17 +233,70 @@ def render_varref(self, node: VarRef, ctx: RenderCtx) -> str:
216233
return _value_to_excel_literal(_value_at_period(node.var._value, ctx.period_idx))
217234

218235
def render_binop(self, node: BinOp, ctx: RenderCtx) -> str:
219-
left = self.walker.render(node.left, ctx)
220-
right = self.walker.render(node.right, ctx)
221236
op = node.op
222-
# // → INT(a/b), % → MOD(a,b) — Excel has no direct operator for these
237+
# // → INT(a/b), % → MOD(a,b). Inside INT/MOD the relevant parent op
238+
# is ``/`` (precedence 3); the comma in MOD separates and needs no
239+
# parens-handling on either operand.
223240
if op == "//":
241+
left = self._render_operand(node.left, "/", ctx, side="left")
242+
right = self._render_operand(node.right, "/", ctx, side="right")
224243
return f"INT({left}/{right})"
225244
if op == "%":
245+
left = self.walker.render(node.left, ctx)
246+
right = self.walker.render(node.right, ctx)
226247
return f"MOD({left},{right})"
248+
249+
left = self._render_operand(node.left, op, ctx, side="left")
250+
right = self._render_operand(node.right, op, ctx, side="right")
227251
excel_op = _PY_TO_EXCEL_OP.get(op, op)
228252
return f"{left} {excel_op} {right}"
229253

254+
def _render_operand(
255+
self, child: Expr, parent_op: str, ctx: RenderCtx, *, side: str
256+
) -> str:
257+
"""Render a BinOp operand and wrap in parens iff Excel precedence
258+
would otherwise re-associate the expression incorrectly.
259+
260+
Wrapping is decided by the *effective* top-level operator of the
261+
rendered string — `_effective_op` follows VarRefs into floating
262+
Variables (whose `_expr` is rendered inline) so we don't miss
263+
compound subtrees that present as VarRef in the AST.
264+
"""
265+
rendered = self.walker.render(child, ctx)
266+
effective = self._effective_op(child)
267+
if effective is None:
268+
return rendered
269+
parent_prec = _BINOP_PRECEDENCE.get(parent_op, 0)
270+
child_prec = _BINOP_PRECEDENCE.get(effective, 0)
271+
if child_prec < parent_prec:
272+
return f"({rendered})"
273+
if (
274+
child_prec == parent_prec
275+
and side == "right"
276+
and parent_op in _NON_COMMUTATIVE_RIGHT
277+
):
278+
return f"({rendered})"
279+
return rendered
280+
281+
def _effective_op(self, node: Expr) -> str | None:
282+
"""The operator that would govern this node's rendered string, or
283+
``None`` if it renders as an atomic token (cell ref, literal,
284+
function call, already-parenthesized subexpr).
285+
286+
Follows VarRefs into floating Variables, since their `_expr` is
287+
rendered inline at the parent's call site.
288+
"""
289+
if isinstance(node, BinOp):
290+
return node.op
291+
if isinstance(node, VarRef):
292+
var = node.var
293+
if var.id in self.addresses:
294+
return None # rendered as a cell address — atomic
295+
if var._expr is not None:
296+
return self._effective_op(var._expr)
297+
return None
298+
return None
299+
230300
def render_subscript(self, node: Subscript, ctx: RenderCtx) -> str:
231301
if not isinstance(node.base, VarRef):
232302
logger.warning(

src/modeleon/display/html.py

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -501,10 +501,10 @@ def _excel_formula_for(var, period_idx: int, ctx: _Ctx) -> Optional[str]:
501501
"{{ background: #fffaf2; color: #07464a; }}"
502502
"{root} label.mo-btn.mo-formula-btn::before "
503503
"{{ content: '\u0192\u00a0'; font-weight: 600; color: #bfa163; margin-right: 2px; }}"
504+
# Label stays "Formulas" in both states — the cream/teal active
505+
# tint plus the brighter gold prefix carry the on/off signal,
506+
# animated by the background/color transition on ``label.mo-btn``.
504507
"{root} label.mo-btn.mo-formula-btn::after {{ content: 'Formulas'; }}"
505-
"{root} input.mo-toggle:checked ~ * label.mo-btn.mo-formula-btn::after, "
506-
"{root} input.mo-toggle:checked ~ label.mo-btn.mo-formula-btn::after "
507-
"{{ content: 'Values'; }}"
508508
# Per-cell click-to-inspect: focusing a formula cell shows its
509509
# formula. Outline uses bright teal so the active cell pops.
510510
"{root} td.mo-cell {{ cursor: pointer; transition: background 80ms ease; }}"
@@ -581,13 +581,13 @@ def _excel_formula_for(var, period_idx: int, ctx: _Ctx) -> Optional[str]:
581581
"{{ border-top: 1px solid #c4cec6; border-radius: 4px; }}"
582582
"{root} label.mo-btn.mo-stack-btn::before "
583583
"{{ content: '\u25a6\u00a0'; font-weight: 600; color: #bfa163; margin-right: 2px; }}"
584+
# Label stays "Stacked" in both states — same convention as the
585+
# Formulas toggle: the active cream/teal tint indicates state,
586+
# animated through the shared ``transition`` on ``label.mo-btn``.
584587
"{root} label.mo-btn.mo-stack-btn::after {{ content: 'Stacked'; }}"
585588
"{root} input.mo-stack:checked ~ * label.mo-btn.mo-stack-btn, "
586589
"{root} input.mo-stack:checked ~ label.mo-btn.mo-stack-btn "
587590
"{{ background: #fffaf2; color: #07464a; }}"
588-
"{root} input.mo-stack:checked ~ * label.mo-btn.mo-stack-btn::after, "
589-
"{root} input.mo-stack:checked ~ label.mo-btn.mo-stack-btn::after "
590-
"{{ content: 'Tabbed'; }}"
591591
)
592592

593593

@@ -599,6 +599,11 @@ def _precedents_script(uid: str) -> str:
599599
the same visual idea as Excel's "Trace Precedents" button. Losing
600600
focus clears the highlight.
601601
602+
Focusing a cell never switches tabs — that would be a navigation
603+
side-effect on what is otherwise a peek action. Precedents on
604+
other tabs still get the ``mo-dep`` class so they're visibly
605+
highlighted whenever the user manually flips to that tab.
606+
602607
Pure-CSS focus-outline and click-to-show-formula already work
603608
without this script; it's a progressive enhancement. If JS is
604609
sandboxed (some JupyterLab configs), the toggle button and

tests/test_excel_golden.py

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -168,12 +168,6 @@ class TestBinOpPrecedenceParens:
168168
accordingly.
169169
"""
170170

171-
@pytest.mark.xfail(
172-
strict=True,
173-
reason="render_binop doesn't wrap a lower-precedence child of "
174-
"multiplication in parens — `a * (1 - b)` collapses to "
175-
"`a * 1 - b` and Excel evaluates as `(a * 1) - b`.",
176-
)
177171
def test_subtraction_inside_multiplication(self, tmp_path):
178172
# ``a * (1 - b)`` — subtraction binds looser than multiplication,
179173
# so the right-hand subtree needs parens. Multiplication has no
@@ -190,12 +184,6 @@ def test_subtraction_inside_multiplication(self, tmp_path):
190184
cells = _cells(tmp_path / "p.xlsx", "S")
191185
assert cells["B3"] == "=B1 * (1 - B2)"
192186

193-
@pytest.mark.xfail(
194-
strict=True,
195-
reason="render_binop doesn't wrap children of `**` (exponent) — "
196-
"`(a/b) ** (1/c)` collapses to `a / b ^ 1 / c`, which "
197-
"Excel reads left-to-right as `((a/b)^1)/c`.",
198-
)
199187
def test_division_and_exponent_groups(self, tmp_path):
200188
# ``(a / b) ** (1 / c)`` — both children of ``**`` are divisions
201189
# which bind tighter than ``**`` in Excel's left-associative

tests/test_html_repr.py

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -168,6 +168,32 @@ def test_cross_tab_formula_uses_qualified_address(self):
168168
# otherwise the cross-tab path silently regressed.
169169
assert "=100.0 * 50" not in html
170170

171+
def test_toolbar_button_labels_stay_stable_across_states(self):
172+
# The Formulas / Stacked toolbar buttons indicate their state
173+
# by colour (cream + teal active tint), not by swapping the
174+
# label to "Values" / "Tabbed". Animation comes from the
175+
# ``label.mo-btn`` transition on background + colour.
176+
m = mo.Model("m")
177+
m.inputs = mo.MultiVariable("Inputs", excel_props={"tab": True})
178+
m.inputs.price = mo.Variable(100, display_name="Price")
179+
m.outputs = mo.MultiVariable("Outputs", excel_props={"tab": True})
180+
m.outputs.revenue = m.inputs.price * mo.Variable(2)
181+
182+
html = m._repr_html_()
183+
184+
import re
185+
186+
formula_contents = re.findall(
187+
r"mo-formula-btn::after \{ content: '([^']+)'", html,
188+
)
189+
stack_contents = re.findall(
190+
r"mo-stack-btn::after \{ content: '([^']+)'", html,
191+
)
192+
# Exactly one content rule per button — no flip to "Values" /
193+
# "Tabbed" lurking anywhere in the emitted CSS.
194+
assert formula_contents == ["Formulas"]
195+
assert stack_contents == ["Stacked"]
196+
171197
def test_each_panel_carries_its_name_for_stacked_mode(self):
172198
# The "Stacked" toolbar toggle hides the tab bar and reveals
173199
# every panel at once — so each panel's title must stay in

0 commit comments

Comments
 (0)