Pin the measure schema block as a shared fixture, and fix the R array bug it exposes - #248
Draft
jat255 wants to merge 6 commits into
Draft
Pin the measure schema block as a shared fixture, and fix the R array bug it exposes#248jat255 wants to merge 6 commits into
jat255 wants to merge 6 commits into
Conversation
jat255
changed the title
jat255
force-pushed
the
jat255/wwmt-schema-fixture
branch
from
September 2, 2026 03:14
6923b6a to
1676857
Compare
|
Preview deployed to Connect ( Deployed from commit 19eb5db. |
|
Preview deployed to Connect ( Deployed from commit 19eb5db. |
jat255
force-pushed
the
jat255/wwmt-schema-fixture
branch
from
September 2, 2026 04:02
1676857 to
6923b6a
Compare
jat255
force-pushed
the
jat255/wwmt-schema-fixture
branch
from
September 2, 2026 22:25
6923b6a to
fc346eb
Compare
An empty-string heading was falling back to the measure name, unlike the R renderer which passes it through. Found by roborev review of 7b16ade.
Covers the fix in 9b5e8ca; both suites already pass it.
measure_schema_text() only recognized pydantic's inline enum key, which Literal produces; a bare enum.Enum schemas as a $ref into $defs and fell back to "string", hiding its vocabulary from the model. Resolve the $ref before inspecting the property or array items. Also narrows as_measure()'s Measure | None through a single test helper instead of nine unchecked accesses, so pyrefly check src tests actually runs clean once the dead ignore-file exclusion is bypassed.
…etail
pydantic wraps every nullable field or array item in an anyOf union
with a {"type": "null"} branch. _argument_detail and _items_label
only looked at the outer node, so a nullable bare enum, nullable
Literal, or nullable enum array all fell through to plain "string",
hiding their vocabulary from the model.
Unwrap anyOf before resolving $ref, in a single _resolve_node the two
callers share, so the same reduction handles nullable and $ref'd
shapes together instead of accreting one branch per shape found.
…ure_schema_text The Python fixture runner hand-built a Measure via create_model(), bypassing measure() and duplicating its model construction; a change to how measure() builds a model could leave the fixture green while R caught it. Generate a real function from each fixture spec and decorate it with @measure instead, matching the R runner's entry point through the production constructor. Also: pass heading only when the fixture case sets one, so R exercises tool_name(td)'s default the way Python's heading=None path already does; rename measure_schema_text's measure parameter to record to stop shadowing the @measure decorator; hoist the repeated set(source_names); and document the supported argument types (scalars, enums, arrays of those) on @measure.
jat255
force-pushed
the
jat255/wwmt-schema-fixture
branch
from
September 3, 2026 00:31
fc346eb to
19eb5db
Compare
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.
Second of four stacked PRs building the Python semantic layer (M3). Based on #247.
search_poolshows the model one block per matching measure. That text is part of what the agent reads, so both packages must emit the same bytes. This addsmeasure_schema_text()and pins the format intests/shared/measure-schema.json, with a runner in each suite.Building the fixture exposed a live bug in the R package, which this PR also fixes. See the R section below for the detail. It is the only reason this PR touches
pkg-r/.Two Python defects surfaced the same way. An explicit
heading=""was treated as absent where R passes it through, and that case is now in the fixture. And an argument typed with a bareenum.Enum, or any nullable argument, rendered asstringwith its vocabulary hidden from the model, because pydantic emits$refandanyOfshapes the renderer did not handle.The enum cases have Python-only tests rather than fixture cases. R has no counterpart to either:
ellmer::type_enum()carries its values inline, andrequired = FALSEmarks a type optional rather than producing a nullable union. There is no cross-language contract to pin.Known gap, left deliberately: a multi-type union such as
Region | OtherEnum | Nonestill renders asstring. No measure in the package constructs one, and a code comment records it.Both fixture runners now enter through their own package's front door. The Python runner previously hand-built the pydantic model and constructed a
Measuredirectly, so it tested rendering against a model the production path never built, while the R runner went throughmeasure(). A change to howmeasure()builds its model would have left the Python runner green, which defeats the point of a shared fixture.R changes
arg_schema_line()renders one argument line of a measure's schema block. Itsarraybranch calledtype_values()on the array's item type.type_values()reads the S7valuesproperty, whichellmer::TypeEnumhas andellmer::TypeBasicdoes not. An array of a basic type therefore did not render poorly, it errored withCan't find property <ellmer::TypeBasic>@values. A newarray_items_label()helper names a basic item type and still lists an enum item's vocabulary. The diff is five lines plus the helper.Behaviour delta. Before, a measure with a
string[]argument raised when its schema was rendered. Now it rendersarray of {string}. Enum arrays are unchanged byte for byte, because the helper reproduces the previous expression for that branch. Nothing else about the line moves.Blast radius.
arg_schema_line()has one caller,measure_schema_text(). That has one caller in the package,search_pool_text(), which backs thesearch_pooltool. So the only reachable effect is that a measure with a non-enum array argument can now appear insearch_pooloutput instead of raising. Reaching it needs a@paramwith astring[]-style type code, whichread_measures()parses intotype_array(items = type_string()).Why it is safe. The enum branch is textually the same expression as before, so no existing snapshot can move. The full package suite passes at 6649 tests with 0 failures. The new fixture runner in
test-measures.Rbuilds a measure from each shared case and compares the rendered text against it. It failed on exactly the string-array case before the fix and passes after it, which is the evidence that the fix targets the reported shape and nothing else.What deserves scrutiny. Whether
type_kind()is the right label for a basic item type. It returns the same words the scalar branch prints, soarray of {string}reads consistently with(string, required), but a reviewer may prefer different wording. Worth a look because the fixture now pins it for both languages.No hand-written R tests were deleted.
pkg-r/tests/testthat/fixtures/shared/measure-schema.jsonis generated byscripts/sync-shared-fixtures.shand needs no review.