feat(gui): take the required marker from the schema's own condition - #7644
Open
kz930 wants to merge 1 commit into
Open
feat(gui): take the required marker from the schema's own condition#7644kz930 wants to merge 1 commit into
kz930 wants to merge 1 commit into
Conversation
A field required only under a condition cannot say so in the top-level `required` array, which is a fixed list, so the marker never appeared for one and the panel gave no sign that a field had become mandatory until the operator was refused. Aggregate needs such a rule and already declares it correctly, under definitions.AggregationOperation.allOf. The marker was then produced a second time, by operator name, in this component. Two statements of one rule, in two languages, free to drift, and the next operator wanting the same thing would have added a third branch beside the field-specific ones already here. conditionalRequiredRules reads what the schema declares. The walk is recursive because a rule may govern a field inside an array item, where it sits under definitions rather than at the top. Keying by field name is enough: the condition is evaluated against the field's own parent model, the row for an array item and the operator for a top-level field, so the same rule resolves correctly in both. It matches on if.properties.<sibling>.const, which the attributeTypeRules blocks do not have, naming their sibling directly and requiring nothing. Aggregate keeps its behaviour and loses its branch. Sklearn's text column, which its descriptor now declares the same way, gets the marker without a line here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Automated Reviewer SuggestionsBased on the
|
Contributor
Author
|
@aglinxinyuan @carloea2 May you take a look at it? |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7644 +/- ##
============================================
- Coverage 89.65% 89.65% -0.01%
Complexity 4397 4397
============================================
Files 1177 1177
Lines 46996 47011 +15
Branches 5268 5273 +5
============================================
+ Hits 42136 42149 +13
Misses 3094 3094
- Partials 1766 1768 +2
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
2 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.
What changes were proposed in this PR?
A field required only under a condition cannot say so in the top-level
requiredarray, which is a fixed list. The property panel drew the marker from that array alone, so the marker never appeared for such a field and the panel gave no sign that a field had become mandatory until the operator was refused.Aggregate needs such a rule and already declares it correctly, under
definitions.AggregationOperation.allOf:attributeis required for every function exceptcount, where an empty attribute means COUNT(*). The marker was then produced a second time, by operator name, in the panel itself. Two statements of one rule, in two languages, free to drift, and the next operator wanting the same thing would have added a third branch beside the field-specific ones already there.conditionalRequiredRulesreads what the schema declares, and the panel applies it. The walk is recursive because a rule may govern a field inside an array item, where it sits underdefinitionsrather than at the top. Keying by field name is enough: the condition is evaluated against the field's own parent model, which is the row for an array item and the operator for a top-level field, so one rule resolves correctly in both. It matches onif.properties.<sibling>.const, which theattributeTypeRulesblocks do not have, naming their sibling directly and requiring nothing.Aggregate keeps its behaviour and loses its branch and helper. An operator that wants a conditional required now states it once, in its descriptor, with no change here.
Any related issues, documentation, discussions?
Closes #7642
How was this PR tested?
Existing tests, plus four cases in
operator-property-edit-frame.component.spec.tscovering thethenform, theelseform nested in a definition as Aggregate states it, anattributeTypeRulesblock that must not be mistaken for one, and a schema stating no condition. These replace the test of the helper that is now removed.ng testpasses: 4465 tests.Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 5)