test: Pin when/unless export reachability for airframe-rx-html migration (#526)#530
Conversation
Issue #526 questioned whether `when` / `unless` are actually in scope through `import wvlet.uni.dom.all.*` after migration from airframe-rx-html. They are: lines 155-156 of all.scala already export them, and the existing "conditional rendering with when" test exercises that path. This change: 1. Adds two `unless` regression tests mirroring the existing `when` ones, so a future refactor of `all.scala` can't silently drop the `unless` export with green tests. 2. Tightens the `Re-export helper functions` doc on `all.scala` to name `when` / `unless` as the airframe-rx-html migration path, so the next reader has the answer in-place without re-running the analysis. No production code change — the exports already work as designed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request addresses issue #526 by confirming and documenting the availability of when and unless helpers within the wvlet.uni.dom.all scope, facilitating migration from airframe-rx-html. The changes include a new design document, enhanced Scaladoc for re-exports, and new regression tests. Feedback was provided to strengthen the test assertions for the unless helper by verifying the specific type and content of the rendered node, ensuring that the presence of empty nodes does not lead to false positive test results.
| test("conditional rendering with unless (false branch renders)"): | ||
| val hidden = false | ||
| val d = div(unless(hidden, span("Visible"))) | ||
| d.modifiers.flatten.size shouldBe 1 |
There was a problem hiding this comment.
The assertion d.modifiers.flatten.size shouldBe 1 is insufficient to verify that the conditional branch actually rendered the expected element. In this framework, DomNode.empty also occupies a slot in the modifiers list, meaning this test would pass even if unless incorrectly returned an empty node. It is better to verify the content or type of the rendered node to ensure the logic is correctly implemented.
d.modifiers.flatten.size shouldBe 1
d.modifiers.flatten.head shouldMatch { case e: DomElement =>
e.name shouldBe "span"
}References
- When type checking, remember that elements from a collection can be of a more specific subtype which may have additional methods not present on the base type.
Addresses codex review on PR #530: the `unless` tests in `wvlet.uni.dom.DomElementTest` cover runtime semantics but not the export contract — they live in `package wvlet.uni.dom`, so `unless` (a top-level def in that same package) is reachable there even if `all.*` stopped re-exporting it. Adds `example.dom.AllExportsTest`, which lives in a sibling package and imports `wvlet.uni.dom.all.*` only. If a future refactor of `all.scala` drops the `when` / `unless` re-export, this file fails to compile. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Re: codex P2 — fixed in 50bef6e. Added Kept the runtime-behavior |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request addresses issue #526 by verifying and documenting that the when and unless helper functions are correctly re-exported and accessible via import wvlet.uni.dom.all.*. The changes include a new regression test in a separate package to ensure export visibility, additional runtime behavior tests for unless, and updated documentation in all.scala regarding the migration path from airframe-rx-html. A review comment correctly identifies that hardcoding line numbers in the plan documentation creates a maintenance burden as those references have already become outdated due to the code changes.
| - `wvlet/uni/dom/all.scala:155-156` exports `wvlet.uni.dom.when` and | ||
| `wvlet.uni.dom.unless`. | ||
| - `uni-dom-test/.../DomElementTest.scala:90-98` already exercises `when` / | ||
| `when false` through `import wvlet.uni.dom.all.*` and passes. |
There was a problem hiding this comment.
Hardcoding specific line numbers in documentation files (e.g., all.scala:155-156) is a maintenance burden as they become outdated whenever the referenced file is modified. In this PR, the line numbers in all.scala have already shifted to 157-158. It is better to refer to the specific symbols or sections by name to ensure the documentation remains accurate over time.
) Addresses gemini-code-assist review on PR #530: 1. The `unless` / `when` "renders" tests asserted `size shouldBe 1`, but `DomNode.empty` also occupies a slot — so the assertion would have passed even if the conditional incorrectly returned empty. Switch to verifying the head modifier is a `DomElement` named "span". 2. Plan doc had hardcoded `all.scala:155-156` references that already shifted to 157-158 from the doc tweak in this PR. Replace with symbol / section names so the plan doesn't bit-rot. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
Closes #526.
The issue asked whether
when/unlessare actually reachable throughimport wvlet.uni.dom.all.*after the airframe-rx-html → uni migration, with two paths: confirm + document, or fix.The exports already work as designed:
wvlet/uni/dom/all.scala:155-156exportswvlet.uni.dom.whenandwvlet.uni.dom.unless.DomElementTest.scala:90-98) already exercises that path throughimport wvlet.uni.dom.all.*.This PR takes the "confirm + document" path:
unlessregression tests so a future refactor ofall.scalacan't silently drop theunlessexport with green tests (today onlywhenwas covered).Re-export helper functionsdoc onall.scalato namewhen/unlessas the airframe-rx-html migration path, so the next reader has the answer in-place.No production code change.
Test plan
./sbt domTest/test— 344/344 pass (was 342, +2 unless tests)../sbt scalafmtAllclean.🤖 Generated with Claude Code