Repository navigation
Preserve formatter fidelity, and get the test suite running again - #3
Open
n-rodriguez wants to merge 11 commits into
Open
n-rodriguez wants to merge 11 commits into
n-rodriguez wants to merge 11 commits into
Conversation
n-rodriguez
changed the title
n-rodriguez
force-pushed
the
fix/formatter-fidelity
branch
from
September 1, 2026 20:02
9640f7b to
5eb14b6
Compare
Both were ways the formatter silently changed the meaning of the code it
re-emitted, found by rewriting Phlex templates where the output is
eval'd: a parenthesised MultiTargetNode flattened into the surrounding
target list, and string literals whose `\` and `#{` were reinterpreted
by the parser. main now fixes both; these cases pin the behaviour.
- test/refract.test.rb: string, symbol and assoc key escaping, block
parameter destructuring, nested multiple-assignment targets
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The suite had not run since the top-level `test` DSL disappeared from Quickdraw: at the top level the name resolves to `Kernel#test`, the file-test builtin, so every file failed to load with `unknown command 'a'` before a single assertion was reached. CI has been red on main since 22 July 2025 for this reason alone. - test/refract.test.rb: wrap the file in a `Quickdraw::Test` subclass, which is how Quickdraw collects tests now - test/refract.test.rb: drop the `assert_equal_ruby` call: the method does not exist — the API is `assert_equivalent_ruby` — and it raised NoMethodError in every test once they could load. The `assert_equal` on the next line is the strict identity check the suite is built on, which subsumes any equivalence check anyway - test/corpus.test.rb: same wrapping; it only loads its test when CORPUS is set, so the failure surfaced only on a corpus run 644 tests pass. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A branch pushed to a fork produced no run of its own: the workflow only triggered on pushes to main and on pull requests targeting main, and a pull request from a fork runs against the upstream repository, where it waits for a maintainer to approve it. `**` rather than `*`, which does not match a slash and so would miss every `fix/…` branch. Tags are left out. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`-t` is not a Quickdraw option, so `bundle exec qt -t 1` aborted before loading a single test file, on every entry of the matrix. Together with the top-level `test` DSL this made two independent reasons for the suite never to run, each hiding the other, and CI has been red on main since 22 July 2025. Passing no flag lets the process count come from `DEFAULT_PROCESSES`, which is 1 where forking is unavailable and the core count otherwise. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`--without` was removed from Bundler: it relied on being remembered across invocations, which Bundler no longer does. The Ruby head jobs carry a version that has dropped it, so both failed before installing a single gem — the last thing standing between this branch and a green matrix, now that the suite itself runs. `bundle config set --local without development` is the documented replacement and has been available since Bundler 2.1, so it covers every entry of the matrix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The matrix stopped at 3.4 plus head, while Phlex — the main consumer of this gem — also covers 4.0 and both TruffleRuby variants. A formatter divergence on a runtime Phlex tests and this gem does not would surface downstream instead of here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three majors behind, and still targeting Node 20: every job logged `actions/checkout@v4 ... being forced to run on Node.js 24`. Version read from the action's latest release, v7.0.1, rather than assumed. `ruby/setup-ruby` is already on its current major. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`inline` records how the node was written, and a rewrite invalidates it.
Replacing the single statement of `foo if bar` with several emitted
`a\nb if bar`, where only the last one stays guarded — the rest runs
unconditionally.
Phlex expands an element into three statements, so a component writing
`span(class: "x") { plain content } if flag` opened the tag and called
`content` even when `flag` was false, leaving the tag unclosed and
running a side effect that should not have happened. 41 failures across
an application's suite came from this, second only to the buffer being
hoisted out of a capture.
All four nodes that carry `inline` have a modifier form and had the same
flaw — `if`, `unless`, `while` and `until` — so the check lives in one
`modifier?` predicate rather than being repeated and forgotten again:
the first version of this fix covered `IfNode` alone, and `unless` was
found afterwards in the same application.
The tests build the nodes by hand: Ruby cannot parse a modifier with
more than one statement, so the shape only ever comes from a rewrite.
`inline_statements?`, which decides whether a statement sits on lines of
its own, read `inline` too: once the modifier is emitted as a block, a
shareable constant among its statements lost its magic comment. It now
asks `modifier?` as well.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`ForwardingSuperNode` pushed its block straight after the keyword, so
`super { … }` came back as `super{ … }`. Ruby accepts it, so this is
fidelity rather than correctness, but every other call with a block is
emitted with the space, `super() { … }` included.
- lib/refract/formatter.rb: emit a space before the block
- test/refract.test.rb: one case
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
n-rodriguez
force-pushed
the
fix/formatter-fidelity
branch
from
October 1, 2026 16:05
fa02f00 to
49dea4d
Compare
Four tests failed on TruffleRuby, for two reasons that CRuby hides. - lib/refract.rb: `Node.attributes` took its order from `Method#parameters`, which lists required keywords first on CRuby and keeps declaration order on TruffleRuby. The order reaches `inspect` and the traversal of children in `shareable_constant_value_within`, so it is now fixed to CRuby's on both - lib/refract/formatter.rb: `escape_string` relied on `[[:print:]]` to decide a high byte of a binary string. CRuby says it is not printable, TruffleRuby says it is, so `"\xFF"` was emitted raw — a source that does not parse. Such bytes are now always escaped - lib/refract/formatter.rb: drop a redundant escape in `BARE_SYMBOL` flagged by Style/RedundantRegexpEscape Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Merged
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
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.
Fixes found while rewriting Phlex templates with Refract, where the output is
eval'd — so a formatting slip becomes a behaviour change rather than a cosmetic one. Rebased onmain, which meanwhile fixed string escaping, destructuring parentheses, empty call parentheses andsuper(); what remains here is whatmaindoes not cover yet, plus the repairs needed to run the suite at all.Formatter
A modifier guarding several statements.
inlinerecords how anif/unless/while/untilwas written, and a rewrite invalidates it: replacing the single statement offoo if barwith several emitteda\nb if bar, where onlybstays guarded and the rest runs unconditionally. Phlex expands an element into three statements, sospan(class: "x") { plain content } if flagopened the tag and rancontenteven whenflagwas false. A singlemodifier?predicate —inlineand exactly one statement — now drives the four visit blocks andinline_statements?, which otherwise dropped theshareable_constant_valuemagic comment of a constant in that body.super { … }came back assuper{ … }. Valid Ruby, but every other call with a block,super() { … }included, is emitted with the space.Test suite and CI
The suite had not run since the top-level
testDSL disappeared from Quickdraw, and CI has been red onmainsince 22 July 2025. Three independent reasons, each hiding the next:bundle exec qt -t 1—-tis not a Quickdraw option, so the runner aborted before loading a file. No flag is needed: the process count comes fromDEFAULT_PROCESSES.testDSL — at the top level the name resolves toKernel#test, the file-test builtin. Tests now live inQuickdraw::Testsubclasses,test/corpus.test.rbincluded, which only failed onceCORPUSwas set.assert_equal_rubydoes not exist — the API isassert_equivalent_ruby. Theassert_equalbeside it is the strict identity check this suite is built on, which subsumes any equivalence check.Also:
bundle install --withoutfails on the Bundler that Ruby head now carries, replaced withbundle config set --local without development; CI runs on every branch, so a fork gets its own run without waiting on approval; and the matrix covers the same Ruby versions as Phlex.Round-trip cases for string, symbol and assoc-key escaping, block-parameter destructuring and nested multiple-assignment targets pin behaviour
mainnow has.Evidence
650 tests pass locally on Ruby 4.0.7, none failing. RuboCop reports a single offense,
Style/RedundantRegexpEscapeinformatter.rb:16, already present onmain.🤖 Generated with Claude Code