fix(phel): restore :phel branch for p/thrown? assert-expr#897
Merged
Conversation
The simplification in jank-lang#888 dropped Phel's reader-conditional branch on the assumption Phel could share the Clojure JVM body. That assumption breaks Phel for two reasons: 1. Argument order. Phel's `phel.test/assert-expr` multimethod is defined as `[form message]` (not Clojure JVM's `[msg form]`), so the shared `:default` body binds the args in the wrong order and the expansion fails to resolve `p/thrown?` at compile time — skipping every test file that uses it. phel-lang main has aligned the signature (phel-lang#2149), but no stable release ships that fix yet. 2. Event-type mismatch. `phel.test/report` dispatches on `:type` and recognises `:pass`/`:failed`/`:error`, not Clojure's `:fail`. The shared body emits `:fail`, which falls through phel's `:default` report method — fanning the event out without touching the counters. Failed `p/thrown?` assertions are silently swallowed and the suite reports a passing run. Re-introduce a `:phel` branch that registers `'p/thrown?` with the correct argument order, catches PHP's `\Throwable`, and reports via `t/report` using `:type :failed`/`:type :pass` so phel's reporter set updates correctly.
Chemaclass
force-pushed
the
fix/phel-thrown
branch
from
7e0d399 to
3fcf6a8
Compare
E-A-Griffin
approved these changes
May 27, 2026
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
3 participants
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.
Summary
#888 assumed Phel could share the Clojure JVM
p/thrown?body. It can't, for two reasons:Arg order. Phel's
phel.test/assert-expris[form message], not[msg form]. The:defaultbody binds args wrong andisfails to resolvep/thrown?at compile time — every test file using it gets skipped. Fixed onphel-langmain(phel-lang/phel-lang#2149) but not yet in a stable release.Event type.
phel.test/reportknows:pass/:failed/:error, not Clojure's:fail.:failevents fall through:defaultwithout touching counters — failedp/thrown?assertions are silently swallowed.Re-add a
:phelbranch: correct arg order, catches\Throwable, reports:type :failed/:pass.Test plan
vendor/bin/phel test test/clojure/string_test/escape.cljc→Passed: 6, Failed: 4, Total: 10(previously skipped at compile time).