forms: recurse one level into nested-aggregate members' schemas - #35
Open
Yaraslaut wants to merge 4 commits into
Open
forms: recurse one level into nested-aggregate members' schemas#35Yaraslaut wants to merge 4 commits into
Yaraslaut wants to merge 4 commits into
Conversation
mergeSchemaExtras previously annotated (x-order, required, title, Quantity/ Choice/widget hints) only an action's own top-level members. A member that was itself a reflectable aggregate -- a nested struct, or std::vector<Sub> -- got none of that on its own sub-members, and its $defs entry got no required array at all, per docs/spec/forms/forms.md's documented "flat actions only" boundary. Domains that are naturally nested (a measurement with a repeated specimen sub-record) had to either flatten the action type (impossible for a repeated sub-record without a fixed max count) or hand-write the form, bypassing the schema-driven generator for exactly the screens that would benefit most from it. Recurse one level into a nested-aggregate member's own schema, applying the same annotation rules the top level already applies (factored into a shared annotateBasicMemberProperty helper). Two schema shapes exist for a nested aggregate, both handled: glaze deduplicates via a shared $defs entry referenced by $ref when the nested type is used two or more times anywhere in the schema, or inlines the object schema directly into the property when it is used exactly once -- annotateNestedAggregateRef resolves whichever form applies before recursing. Deliberately capped at exactly one level, matching the design doc's own suggested bound: a nested aggregate's own nested-aggregate members are left unannotated, and computed fields/formLayout/fieldSpans/formRules stay top-level-only. Purely additive -- an action with no nested-aggregate member has nothing to trigger on, so its schema is byte-for-byte unchanged (the full existing suite, including every prior schema-generation test, passes unmodified). Render-side add/remove affordances for a repeated-aggregate array are a separate, downstream renderer concern the issue explicitly called out as distinct from the annotation-reach gap this closes, and are out of scope here. Closes #25 Signed-off-by: Yaraslau Tamashevich <yaraslau.tamashevich@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
The nested-aggregate annotation pass (issue #25) shares annotateBasicMemberProperty with the top-level pass, so it applies the same FieldMeta/Quantity/Choice/widget/ranged-bounds rules to a nested member's own properties -- but every existing nested-forms fixture (Specimen, Attachment, Provenance) only has plain scalar members, so those branches were never exercised through the nested path. Add RichSub/RichRecord, a nested type whose own members carry a FieldMeta override, a Quantity, and a Choice, and assert the resolved nested schema carries the expected x-placeholder/ x-readonly/x-hidden/x-decimalPlaces/x-optionsAction annotations. Signed-off-by: Yaraslau Tamashevich <yaraslau.tamashevich@gmail.com>
The RichSub/RichRecord coverage fixture's UnitTraits<NestedFormUnit>::meta() switched on `unit` with only a `kg` case plus `default`, which trips -Wswitch-enum (part of -Weverything, and not in apply_warnings()'s opt-out list) since it requires every enumerator named regardless of a default label. Broke all 15 real build/test CI jobs sharing this compile step. Add an explicit `case NestedFormUnit::scalar:` falling through to the existing default body (kept for defensiveness; -Wcovered-switch-default is already suppressed project-wide). Signed-off-by: Yaraslau Tamashevich <yaraslau.tamashevich@gmail.com>
codecov/patch still failed (79.41%, target 97.11%) after the previous coverage-fix commit. Measured actual llvm-cov line coverage locally (-fprofile-instr-generate/-fcoverage-mapping, matching the CI clang-coverage job) rather than guessing further from codecov's diff report, which does not map cleanly onto raw added-line numbers. RichSub's FieldMeta entry and Quantity field left three `annotateBasicMemberProperty` branches (in the nested nested-aggregate path this PR adds) permanently untaken: FieldMeta::i18nKey (unset), FieldMeta::widget (unset), and Quantity::unitAlternatives() (empty -- NestedFormUnit declared no relations). Add a g<->kg relation and set i18nKey/widget on the existing fixture, and assert on x-i18nKey/x-widget/x-unitAlternatives at the nested level. Verified locally: all previously-0-hit added lines now execute at least once, full suite (819 cases) still passes. Signed-off-by: Yaraslau Tamashevich <yaraslau.tamashevich@gmail.com>
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.
Summary
mergeSchemaExtraspreviously annotated (x-order,required,title,Quantity/Choice/widget hints) only an action's own top-level members. A member that was itself a reflectable aggregate — a nested struct, orstd::vector<Sub>— got none of that on its own sub-members, matching the issue's exact repro (Specimen/replicateslanding in$defswith no annotations and norequired).annotateBasicMemberPropertyhelper so both the top-level pass and the nested pass apply identical rules).$defsentry referenced by$refwhen the nested type is used 2+ times anywhere in the schema, or the object schema inlined directly into the property when it's used exactly once.annotateNestedAggregateRefresolves whichever form applies before recursing.formLayout/fieldSpans/formRulesremain top-level-only — matching the depth-limit and scope suggestions in the issue itself.docs/spec/forms/forms.mdupdated: replaces the old "flat actions only" scope note with a new "Nested aggregates (one level)" section describing both schema shapes, the depth cap, and what stays top-level-only.Test plan
tests/test_nested_forms.cpp: flat top-level fields unaffected; single nested member andstd::vector<Sub>member annotated correctly in both the deduplicated ($ref/$defs) and inlined forms; depth cap (level-2 nesting stays unannotated); idempotence when two members share the same nested type../build/tests/morph_tests— all 818 test cases / 8347 assertions pass, including every pre-existing schema-generation test (test_quantity_forms.cpp,test_forms_conformance_corpus.cpp, etc.) unmodified.Closes #25
🤖 Generated with Claude Code