-
Notifications
You must be signed in to change notification settings - Fork 3
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
Allow vanilla (de)serialization of discriminated unions #132
Conversation
dbc2615
to
5d9ff6b
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #132 +/- ##
==========================================
- Coverage 96.31% 96.25% -0.06%
==========================================
Files 9 9
Lines 949 935 -14
==========================================
- Hits 914 900 -14
Misses 35 35 ☔ View full report in Codecov by Sentry. |
0fa2ccf
to
57f627e
Compare
Current handling requires that subclasses of type annotated with Types that want to have their schema updated when a new instance of a class decorated with The FastAPI schema for the routes into the service does not update when a new subclass is created, nor do any types that refer to types that refer to a discriminated union. If Pydantic exposed some way of propagating that a schema that is used in the schema of a model has updated, so the schema of the model must also update, then |
I've made an issue for that last part, but I don't think it will gain much traction as I think creating runtime models often goes against the purpose of validation |
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, minor points only, thanks for this!
src/scanspec/core.py
Outdated
@@ -104,134 +102,106 @@ def calculate(self) -> int: | |||
) | |||
|
|||
Args: | |||
super_cls: The superclass of the union, Expression in the above example | |||
cls: The superclass of the union, Expression in the above example |
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.
cls: The superclass of the union, Expression in the above example | |
super_cls: The superclass of the union, Expression in the above example |
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, Thank you Joseph
Fixes #130, #131