label-studio
label-studio copied to clipboard
fix: LEAP-634: Actualize label config validation
Config validation didn't take into account many tags and their attributes. This PR might fix this.
It also fixes nested Choice validation.
It also will fix validation messages. (Usually there is no path at all and sometimes it doesn't tell about the problem itself)
PR fulfills these requirements
- [ ] Tests for the changes have been added/updated
- [ ] Docs have been added/updated
- [x] Best efforts were made to ensure docs/code are concise and coherent (checked for spelling/grammatical errors, commented out code, debug logs etc.)
- [x] Self-reviewed and ran all changes on a local instance
Explain the changes you've made
- Changing
exc.pathintoexc.absolute_pathfixes displaying validation paths fromValidation failed on :toValidation failed on View/Labels/Label:. - Changing
{
"anyOf": [
{
"type": "array",
"items": {"$ref": "#/definitions/ref1"}
},
{
"$ref": "#/definitions/ref1"
}
]
}
into
{
"anyOf": [
{
"$ref": "#/definitions/ref1"
},
{
"type": "array",
"items": {"$ref": "#/definitions/ref1"}
}
]
}
fixes displaying cause of validation fails from OrderedDict() is not of type 'array' to something more meaningful like 'value' is a required property
- The line
# config = _fix_choices(config)
was hidden under the comment due to the problem, which caused this workaround, was solved by json schema itself.
What alternative approaches were there?
Fixing displayed paths has leaded to the situation when
<View>
<Text name="text" value="$text" />
<Choices name="choice" toName="choice">
<Choice />
<Choice value="any" />
</Choices>
</View>
shows View/Choices/Choice/0, but
<View>
<Text name="text" value="$text" />
<Choices name="choice" toName="choice">
<Choice />
</Choices>
</View>
shows View/Choices/Choice. And previously it was just Choice/0 in this case.
To make it more consistent we can use removed right now function _fix_choices but convert all tags into arrays and in the same time get rid of MaybeMultiple* rules in schema. But so far it doesn't look to me that it's necessary, 'cause doesn't really affects user's navigation.
This change affects (describe how if yes)
- [ ] Performance
- [ ] Security
- [ ] UX
Does this PR introduce a breaking change?
- [ ] Yes, and covered entirely by feature flag(s)
- [ ] Yes, and covered partially by feature flag(s)
- [ ] No
- [x] Not sure (briefly explain the situation below)
It depends on the validity of the validation rules.
What level of testing was included in the change?
- [ ] e2e (codecept)
- [ ] integration (cypress)
- [ ] unit (jest)
Which logical domain(s) does this change affect?
Labeling Interface
Deploy Preview for heartex-docs ready!
| Name | Link |
|---|---|
| Latest commit | 4c0521b3682e6b304e4e169f3de2bc8bd3a8d6c2 |
| Latest deploy log | https://app.netlify.com/sites/heartex-docs/deploys/65b9c4c175233900080212d5 |
| Deploy Preview | https://deploy-preview-5313--heartex-docs.netlify.app/guide/billing |
| Preview on mobile | Toggle QR Code...Use your smartphone camera to open QR code link. |
To edit notification comments on pull requests, go to your Netlify site configuration.
Deploy Preview for label-studio-docs-new-theme ready!
| Name | Link |
|---|---|
| Latest commit | 4c0521b3682e6b304e4e169f3de2bc8bd3a8d6c2 |
| Latest deploy log | https://app.netlify.com/sites/label-studio-docs-new-theme/deploys/65b9c4c13df3b30008064e90 |
| Deploy Preview | https://deploy-preview-5313--label-studio-docs-new-theme.netlify.app |
| Preview on mobile | Toggle QR Code...Use your smartphone camera to open QR code link. |
To edit notification comments on pull requests, go to your Netlify site configuration.
Codecov Report
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 75.79%. Comparing base (
4e0f517) to head (4c0521b). Report is 326 commits behind head on develop.
Additional details and impacted files
@@ Coverage Diff @@
## develop #5313 +/- ##
===========================================
- Coverage 75.88% 75.79% -0.10%
===========================================
Files 154 154
Lines 12931 12930 -1
===========================================
- Hits 9813 9800 -13
- Misses 3118 3130 +12
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
I will say we probably don't want to check in a commented-out function call; one way forward would be to remove the line and add a comment like
# previously we called `_fix_choices` here, but no longer need to because...
@jombooth Thanks for that! It looks much better.
This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.