Fixes to union properties #241
Conversation
When calling out to the template of an inner property
Reduce indent of inner properties Don't declare type on call-out to subtemplate
|
CC @dbanty Also the test failures are coming from the |
Codecov Report
@@ Coverage Diff @@
## main #241 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 46 46
Lines 1302 1302
=========================================
Hits 1302 1302 Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Awesome work, thanks! Those template changes should help along our eventual goal of having a stable template "API" for custom templates.
There's one change in the enum template that I think needs to be reverted.
As for the custom-e2e stuff, it doesn't get regenerated with the normal poetry run task regen, I made it a separate poetry run task regen_custom when testing the initial custom template stuff. Though there's really no reason that can't just be part of regen. If you run poetry run task re it will regen both and run the e2e test.
| elif isinstance(self.a_property, AnEnum): | ||
| a_property = UNSET | ||
| if not isinstance(self.a_property, Unset): | ||
| a_property = self.a_property |
There was a problem hiding this comment.
This is weird, I'll try to find the place in the template that causes it. Obviously no need to double check isinstance on the same property, we already know it's AnEnum.
There was a problem hiding this comment.
I don't see where your changes would have caused this, so it was probably an existing issue with Unions/UNSET (I know there is at least one issue related to that). The inner_property is probably being set as not required which makes the inner constructor redo the check. I can handle that in a future version.
| @staticmethod | ||
| def from_dict(d: Dict[str, Any]) -> "ModelWithUnionProperty": | ||
| def _parse_a_property(data: Any) -> Union[Unset, AnEnum, AnIntEnum]: | ||
| data = None if isinstance(data, Unset) else data |
There was a problem hiding this comment.
Why None this instead of checking for Unset further down? Less template code when nullables are involved maybe?
There was a problem hiding this comment.
Yeah, this was to be able to reuse construct from template for the inner property without further modifying.
Most of the constructs just check if {{ source }} is not None and don't account for UNSETs.
I'll look a bit more to see if there is a better way.
There was a problem hiding this comment.
If there's not an easy fix it's not a huge deal. If you want to just regen the custom template e2e test so the checks pass I'll merge this.
| {{ destination }} = {{ source }} if {{ source }} else None | ||
| {% else %} | ||
| {{ destination }} = {{ source }}.value | ||
| {{ destination }} = {{ source }} | ||
| {% endif %} |
There was a problem hiding this comment.
I think the .values here are still necessary when converting this to JSON, unless HTTPX automatically converts Enums to their values?
There was a problem hiding this comment.
This was a typing issue.
Mypy complained when I generated the model_with_union_property
a_property: Union[Unset, AnEnum, AnIntEnum]
try:
a_property = UNSET
if data is not None:
a_property = AnEnum(data).value
# ...
There was a type error, saying I was providing a str to a var typed as Union[Unset, AnEnum, AnIntEnum]
FWIW, json.dumps does convert these enums to their primitive values (both (str, Enum) and IntEnum types). I'm assuming that's what httpx uses
There was a problem hiding this comment.
Oh I forgot that we inherit from (str, Enum) instead of just Enum for this exact reason. So this is great then!
transforms oninner_propertys of theUnionPropertytransformon all templates to take in adeclare_typekwarg (defaultTrue) that determines if the first declaration of the property should include typedefunion_property.pyi'stransformto prevent mypy issues when calling thetransformof itsinner_propertiesconstructon all templates to take ainitial_valuekwarg (defaults as appropriate, usuallyNone,[]for lists)union_property.pyi'sconstructto prevent invalidly assigningNoneto theinner_property_parse_<inner_property>function inunion_property'sconstructto use the passed subresource instead of the top level dict from the outer scope