-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(symbol sources): Correct schema and validate builtin sources #87267
base: master
Are you sure you want to change the base?
Conversation
Second attempt of #86878. Validating the builtin symbol sources against the schema revealed that it was incorrect in several ways (missing file types, filters, `is_public`). I also added the validation of the builtin sources as a test so that in the future a mistake like #86873 fails in CI. Unlike #86878, this temporarily keeps around an incorrect piece of the old schema for backwards compatibility. It will be reomved when the sources using it have been fixed.
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #87267 +/- ##
========================================
Coverage 87.73% 87.74%
========================================
Files 9865 9862 -3
Lines 558942 558805 -137
Branches 22041 22016 -25
========================================
- Hits 490403 490320 -83
+ Misses 68108 68078 -30
+ Partials 431 407 -24 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, left a question
COMMON_SOURCE_PROPERTIES = { | ||
"id": {"type": "string", "minLength": 1}, | ||
"name": {"type": "string"}, | ||
"layout": LAYOUT_SCHEMA, | ||
"filters": FILTERS_SCHEMA, | ||
# FIXME: This is temporarily included for backwards compatibility. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if we send a payload without filetypes
? Will this fail to validate?
My worry is that once we fade out the old field, this code will fail validation.
Second attempt of #86878.
Validating the builtin symbol sources against the schema revealed that it was incorrect in several ways (missing file types, filters,
is_public
).I also added the validation of the builtin sources as a test so that in the future a mistake like #86873 fails in CI.
Unlike #86878, this temporarily keeps around an incorrect piece of the old schema for backwards compatibility. It will be removed when the sources using it have been fixed.