Refactor to LET — PR 1: tracer (spec 0008)#206
Merged
Conversation
Spec defines the /Refactor slash command and the four-PR delivery slicing. Plan details engine/UI split, reuse from /Gather, and the PR-by-PR scope. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
FormulaRef gains a bool IsSpilled field with equality + hash + DisplayAddress rendering so A1 and A1# dedupe as distinct refs — spec 0008's /Refactor needs them as separate LET bindings. CellRefExtractor.BuildFormulaRef populates the flag from the existing (?<spill>#) regex group. /Gather is preserved through two seams: CellGraphWalker normalises the spill flag out of cell-level precedents (graph identity already lives on CellRef, and spill-ness on WalkedCell.HasSpill), and CellRefExtractor.Rewrite falls back from a spilled key to its non-spilled equivalent when the spilled key isn't in the lookup — so /Gather's existing PR 5 behaviour of collapsing A1# tokens to the anchor's binding name still holds. The two existing CellRefExtractor tests that pinned the old "A1 and A1# dedupe to one ref" rule are updated to reflect the new spec'd behaviour. A new CellRefExtractorSpillTests file covers the IsSpilled propagation and the two rewriter paths. All 908 existing tests stay green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
End-to-end loop on the simplest case: a non-LET formula with cell refs and ranges (including spill refs). The author runs /Refactor on the active cell, picks names + reorders in the dialog, and saves — the synthesised =LET(...) replaces the cell formula via Formula2. Engine (RefactorEngine + RefactorTypes): - Refactor(formula, activeSheet) extracts via CellRefExtractor.Extract, dedupes by FormulaRef (which now distinguishes spill from anchor), assigns inputN auto-names, rewrites via CellRefExtractor.Rewrite, and emits the synthesised LET via FormulaFormatter.AppendLet. - Recompute(formula, activeSheet, rows) re-runs with the dialog's per-row state — rename, Include drop, reorder. - Existing LETs refuse with a "coming in PR 2" diagnostic. PR 2 removes this branch. UI (RefactorToLetWindow): - Read-only original-formula header, inputs list with Include checkbox, rename TextBox, RHS preview, drag/Alt-Up-Down reorder, and a live preview pane. Save returns the synthesised LET via SavedFormula. - Per-row name validation via ExcelNameValidator plus cross-row uniqueness; Save is disabled until every included row is valid. Slash command (RefactorCommand): - Reads the active cell's formula via Formula2 (dynamic-array safe), runs the engine, opens the dialog on the popup UI thread, and writes the saved LET back. Mirrors GatherCommand's threading + diagnostic pattern. - Registered in LambdaPopup.BuildCommandRegistry() under /Refactor. Tests (RefactorEngineTests): single ref, multi-ref dedupe, range, spill-distinct, sheet-qualifier dedupe, cross-sheet distinct, refused-LET, no-refs literal, rename, Include drop, reorder, the spec's worked example, and a round-trip helper that asserts every synthesised LET parses via LetParser AND feeds cleanly through LetToLambdaBuilder. All 921 tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Nullable warnings (4 sites): net48's BCL doesn't carry the
[NotNullWhen(false)] annotation on string.IsNullOrEmpty /
IsNullOrWhiteSpace, so the analyzer can't narrow after those guards.
The parameters legitimately need to stay nullable (public/defensive
APIs that accept workbook input). Fix at the access site via the
canonical null-forgiving idiom — either `value!` after the guard, or
an `is { } local` pattern-match where it reads more naturally:
- EditLambdaCommand.TryParseLambdaCall: formula! at FindMatchingClose
- ExcelNameValidator.Validate: re-bind to a non-null local `n`
- LetNameSanitizer.Sanitize: text!.Trim()
- GatherEngine: r.NameOverride is { Length: > 0 } nameOverride
Redundant coalesce: CellRef.GetHashCode coalesced Sheet to "" before
hashing as a defensive measure for Microsoft.Bcl.HashCode's net48
null-intolerance. Sheet is annotated non-nullable, so the coalesce
is dead code — kept the ExternalWorkbook coalesce (genuinely nullable).
Polyfill namespace warning: NetFrameworkPolyfills.cs deliberately
lives in `System.Collections.Generic` so the extension method is
discoverable wherever that namespace is in scope (implicit via
Directory.Build.props). Added `// ReSharper disable once CheckNamespace`
plus a comment explaining the intent.
Unused fields: RefactorToLetWindow stored _activeSheet and
_originalFormula in fields but never read them — the constructor
already pipes them straight into the UI controls and the recompute
closure captures activeSheet via the wrapping command. Dropped both
fields and the activeSheet constructor parameter; updated the
RefactorCommand call-site accordingly.
All 921 tests still green.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #202
Tracer-bullet slice of spec 0008 —
/Refactor. End-to-end loop on the simplest case: a non-LET formula with cell refs and ranges (including spill refs), the dialog with rename / Include / reorder, and write-back viaFormula2. Existing LETs are refused with a "coming in PR 2" message.Summary
FormulaRefgainsIsSpilled(equality + hash +DisplayAddress) soA1andA1#dedupe as distinct refs.CellRefExtractor.BuildFormulaRefpopulates it;CellRefExtractor.Rewritefalls back from spilled → non-spilled when the spilled key isn't in the lookup, preserving/Gather's PR 5 token-collapse behaviour.CellGraphWalkernormalises the spill flag out of cell-level precedents (graph identity already lives onCellRef; spill-ness onWalkedCell.HasSpill).RefactorEngine.Refactor(formula, activeSheet)returns auto-named bindings + the synthesised LET.Recompute(...)re-runs with per-row state (rename / Include drop / reorder).RefactorToLetWindow— read-only formula header, inputs list with Include checkbox + rename + drag/Alt-Up-Down reorder, live preview, Save/Cancel. Per-row + cross-row name validation gates the Save button./Refactorregistered inLambdaPopup;RefactorCommandmirrorsGatherCommand's threading + diagnostic pattern, writes back viaFormula2.Test plan
dotnet build addin/lambda-boss.slnx— cleandotnet test addin/lambda-boss.Tests— 921 passing (up from 900: 13 newRefactorEngineTests+ 8 newCellRefExtractorSpillTests)=IF(A1<10, IF(A1>2, SUM(B1:B5), 0), SUM(B2:B6))→/Refactor→ Save — confirmed by Tim that the resulting LET evaluates identicallyReference
specs/0008-refactor-to-let.mdplans/0008-refactor-to-let.md§ PR 1🤖 Generated with Claude Code