|
| 1 | +**Re-Review: Liquid `let` tag -- Commit 56c35dfb** |
| 2 | + |
| 3 | +## Precedent Gathered |
| 4 | + |
| 5 | +### Reference Files |
| 6 | +| File | Why Selected | Key Patterns | |
| 7 | +|------|--------------|--------------| |
| 8 | +| `/Users/karreiro/src/github.com/Shopify/liquid/lib/liquid/tags/assign.rb` | Closest sibling tag (variable assignment) | YARD `@liquid_public_docs` block, `Syntax` constant, `assign_score_of`, `ParseTreeVisitor` | |
| 9 | +| `/Users/karreiro/src/github.com/Shopify/liquid/lib/liquid/tags/capture.rb` | Block-scoped variable tag | YARD docs, `Block#render` indirection, `has_let?` gating | |
| 10 | +| `/Users/karreiro/src/github.com/Shopify/liquid/lib/liquid/tags/if.rb` | Block tag with `render_body` pattern | `render_body` helper, `has_let?` check, `context.stack` | |
| 11 | +| `/Users/karreiro/src/github.com/Shopify/liquid/lib/liquid/tags/case.rb` | Block tag with `render_body` pattern | Identical `render_body` to `if.rb` | |
| 12 | +| `/Users/karreiro/src/github.com/Shopify/liquid/.rubocop_todo.yml` | RuboCop exclusions registry | Alphabetical ordering within `Naming/ConstantName` | |
| 13 | +| `/Users/karreiro/src/github.com/Shopify/liquid/test/unit/parse_tree_visitor_test.rb` | Existing `Style/SymbolProc` disable pattern | Line 269: `# rubocop:disable Style/SymbolProc` | |
| 14 | + |
| 15 | +--- |
| 16 | + |
| 17 | +## Verification of Claimed Fixes |
| 18 | + |
| 19 | +### Finding #1: YARD Documentation on `let.rb` |
| 20 | + |
| 21 | +**Precedent:** `/Users/karreiro/src/github.com/Shopify/liquid/lib/liquid/tags/assign.rb:4-19` uses the `@liquid_public_docs` block with `@liquid_type`, `@liquid_category`, `@liquid_name`, `@liquid_summary`, `@liquid_description`, `@liquid_syntax`, and `@liquid_syntax_keyword` annotations, placed immediately before the class declaration. |
| 22 | + |
| 23 | +**New code:** `/Users/karreiro/src/github.com/Shopify/liquid/lib/liquid/tags/let.rb:4-17` now has a `@liquid_public_docs` block with the same annotation structure: `@liquid_type tag`, `@liquid_category variable`, `@liquid_name let`, `@liquid_summary`, `@liquid_description`, `@liquid_syntax`, and `@liquid_syntax_keyword` entries. |
| 24 | + |
| 25 | +**Verdict:** Consistent. The annotation ordering matches `assign.rb` and `capture.rb`. Category is `variable`, matching the other variable-family tags (`assign`, `capture`). |
| 26 | + |
| 27 | +--- |
| 28 | + |
| 29 | +### Finding #2: `.rubocop_todo.yml` Exclusion for `let.rb` |
| 30 | + |
| 31 | +**Precedent:** `/Users/karreiro/src/github.com/Shopify/liquid/.rubocop_todo.yml:84-100` lists `Naming/ConstantName` exclusions in alphabetical order: `assign.rb`, `capture.rb`, `case.rb`, `cycle.rb`, `for.rb`, `if.rb`, ... `raw.rb`, `table_row.rb`. |
| 32 | + |
| 33 | +**New code:** `/Users/karreiro/src/github.com/Shopify/liquid/.rubocop_todo.yml:94` inserts `lib/liquid/tags/let.rb` between `if.rb` (line 93) and `raw.rb` (line 95). |
| 34 | + |
| 35 | +**Verdict:** Consistent. Alphabetical ordering is preserved. |
| 36 | + |
| 37 | +--- |
| 38 | + |
| 39 | +### Finding #6: `capture.rb` Else Branch -- `render(context)` Indirection |
| 40 | + |
| 41 | +**Precedent:** `/Users/karreiro/src/github.com/Shopify/liquid/lib/liquid/block.rb:20-22` defines `Block#render(context)` as a backwards-compatibility wrapper that delegates to `@body.render(context)`. The original `capture.rb` used `render(context)` in its else branch prior to the `let` feature, preserving this indirection layer. |
| 42 | + |
| 43 | +**New code:** `/Users/karreiro/src/github.com/Shopify/liquid/lib/liquid/tags/capture.rb:39` uses `render(context)` in the else branch (when `has_let?` is false), while the `has_let?` branch on line 37 uses `context.stack { @body.render(context) }`. |
| 44 | + |
| 45 | +**Verdict:** Consistent. The else branch preserves the `Block#render` indirection for the non-`let` path, matching the original behavior. The `has_let?` branch correctly adds `context.stack` wrapping. |
| 46 | + |
| 47 | +**Note:** The `if.rb` and `case.rb` tags use a different pattern for their else branches -- they call `body.render_to_output_buffer(context, output)` directly. This difference is expected because `capture.rb` needs to capture output as a string (via `@body.render(context)` which returns a string), while `if.rb`/`case.rb` write directly to an output buffer. The patterns are structurally different for a valid reason. |
| 48 | + |
| 49 | +--- |
| 50 | + |
| 51 | +### RuboCop Fix: `Style/RegexpLiteral` in `let_tag_test.rb` |
| 52 | + |
| 53 | +**New code:** `/Users/karreiro/src/github.com/Shopify/liquid/test/integration/tags/let_tag_test.rb:125` uses `%r{</tr>\d}` instead of `/<\/tr>\d/`. |
| 54 | + |
| 55 | +**Verdict:** Consistent with RuboCop's `Style/RegexpLiteral` rule. The `%r{}` form is the correct way to avoid escaping forward slashes. |
| 56 | + |
| 57 | +--- |
| 58 | + |
| 59 | +### RuboCop Fix: `Style/SymbolProc` Disable in `let_tag_unit_test.rb` |
| 60 | + |
| 61 | +**Precedent:** `/Users/karreiro/src/github.com/Shopify/liquid/test/unit/parse_tree_visitor_test.rb:269` uses the identical pattern: |
| 62 | +```ruby |
| 63 | +.add_callback_for(VariableLookup) { |node| node.name } # rubocop:disable Style/SymbolProc |
| 64 | +``` |
| 65 | + |
| 66 | +**New code:** `/Users/karreiro/src/github.com/Shopify/liquid/test/unit/tags/let_tag_unit_test.rb:61` uses: |
| 67 | +```ruby |
| 68 | +.add_callback_for(VariableLookup) { |node| node.name } # rubocop:disable Style/SymbolProc |
| 69 | +``` |
| 70 | + |
| 71 | +**Verdict:** Identical to precedent. The disable comment is justified: `add_callback_for` invokes the block with `(node, context)`, so `&:name` would pass both arguments to `Symbol#to_proc`, which only expects `self`. The block form is necessary. |
| 72 | + |
| 73 | +--- |
| 74 | + |
| 75 | +## Non-Blocking Items (Confirmed Acknowledged) |
| 76 | + |
| 77 | +| Finding | Reason Not Changed | Assessment | |
| 78 | +|---------|--------------------|------------| |
| 79 | +| #3: `assign_score_of` duplication | No extraction pattern exists in codebase | Correct -- duplication matches `assign.rb` exactly | |
| 80 | +| #4: `has_let?` naming | `Naming/PredicatePrefix` disabled in `.rubocop_todo.yml:183-184` | Correct -- not enforceable | |
| 81 | +| #5: `render_body` duplication in `if.rb`/`case.rb` | No extraction pattern exists | Correct -- both tags define identical private methods | |
| 82 | +| #7: Category separator comments in tests | Cosmetic; no enforced convention | Correct -- harmless | |
| 83 | + |
| 84 | +--- |
| 85 | + |
| 86 | +## Findings |
| 87 | + |
| 88 | +| Deviation | Status | |
| 89 | +|-----------|--------| |
| 90 | +| Finding #1 (YARD docs) | **Resolved** -- matches `assign.rb`/`capture.rb` pattern | |
| 91 | +| Finding #2 (`.rubocop_todo.yml`) | **Resolved** -- alphabetically correct | |
| 92 | +| Finding #6 (`render(context)` indirection) | **Resolved** -- preserves `Block#render` wrapper | |
| 93 | +| RuboCop `Style/RegexpLiteral` | **Resolved** -- `%r{}` form correct | |
| 94 | +| RuboCop `Style/SymbolProc` | **Resolved** -- matches `parse_tree_visitor_test.rb:269` | |
| 95 | + |
| 96 | +**Total remaining deviations from established patterns:** 0 |
| 97 | + |
| 98 | +--- |
| 99 | + |
| 100 | +## Ambiguities |
| 101 | + |
| 102 | +None -- patterns are consistent. |
| 103 | + |
| 104 | +--- |
| 105 | + |
| 106 | +## Verdict |
| 107 | + |
| 108 | +**APPROVED.** |
| 109 | + |
| 110 | +All blocking deviations from the original review have been addressed. Each fix is verified against existing codebase precedent. The non-blocking items were correctly identified as acceptable and left unchanged. Test suite passes with 0 failures and RuboCop reports 0 offenses. |
0 commit comments