-
-
Notifications
You must be signed in to change notification settings - Fork 261
Fix deserialization of unions #332
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
Changes from 1 commit
4e30205
fda721a
10926f1
465943d
9ec7394
db33752
49185f0
b71f718
c251560
0d79ce8
3c536f7
8a72732
b5fa15d
43969ae
2c4fdf8
caea77a
da1d970
682c5ab
39faebf
55496e5
1ddd486
8bc76af
0d90970
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,7 +15,7 @@ if {% if property.nullable %}_{{ property.python_name }} is not None{% endif %}{ | |
| {% endif %} | ||
| {% endmacro %} | ||
|
|
||
| {% macro check_type_for_construct(source) %}isinstance({{ source }}, dict){% endmacro %} | ||
| {% macro check_type_for_construct(property, source) %}isinstance({{ source }}, dict){% endmacro %} | ||
|
|
||
| {% macro transform(property, source, destination, declare_type=True) %} | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We probably need a better name for this macro than "transform" at some point 😅even I can't remember what this one is for sometimes. Not a note on this PR, just a note on how I need to put some design thought into the templates.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Haha, yeah...maybe
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. or |
||
| {% if property.required %} | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,15 +13,15 @@ def _parse_{{ property.python_name }}(data: {{ property.get_type_string(json=Tru | |
| {% if not loop.last or property.has_properties_without_templates %} | ||
| try: | ||
| {% from "property_templates/" + inner_property.template import construct, check_type_for_construct %} | ||
| if not {{ check_type_for_construct("data") }}: | ||
| if not {{ check_type_for_construct(inner_property, "data") }}: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In theory we could start doing: and avoid the try/except stuff, yeah?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think it would work out-of-the-box, because we're also catching non-type errors, like parsing dates ( |
||
| raise TypeError() | ||
| {{ construct(inner_property, "data", initial_value="UNSET") | indent(8) }} | ||
| return {{ property.python_name }} | ||
| except: # noqa: E722 | ||
| pass | ||
| {% else %}{# Don't do try/except for the last one #} | ||
| {% from "property_templates/" + inner_property.template import construct, check_type_for_construct %} | ||
| if not {{ check_type_for_construct("data") }}: | ||
| if not {{ check_type_for_construct(inner_property, "data") }}: | ||
| raise TypeError() | ||
| {{ construct(inner_property, "data", initial_value="UNSET") | indent(4) }} | ||
| return {{ property.python_name }} | ||
|
|
@@ -36,7 +36,7 @@ def _parse_{{ property.python_name }}(data: {{ property.get_type_string(json=Tru | |
| {% endmacro %} | ||
|
|
||
| {# For now we assume there will be no unions of unions #} | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the future we should probably be unrolling unions while building the properties. I don't imagine that would be too difficult, just a check on each inner_property to see if it's a UnionProperty.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, yeah, that makes sense. |
||
| {% macro check_type_for_construct(source) %}True{% endmacro %} | ||
| {% macro check_type_for_construct(property, source) %}True{% endmacro %} | ||
|
|
||
| {% macro transform(property, source, destination, declare_type=True) %} | ||
| {% if not property.required or property.nullable %} | ||
|
|
||
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.