Better compatibility for "required" vs. "nullable" #230

emann · 2020-11-02T23:12:12Z

Big thanks to @bowenwr for his groundwork for the UNSET stuff! I took a lot of inspiration from his fix + added further separation of required vs. nullable to get around some of the edge cases such as query & path params.

Closes #205, #228

…uired and nullable. Model to_dict methods now have parameters to alter what fields are included/excluded

codecov · 2020-11-02T23:13:42Z

Codecov Report

Merging #230 into main will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #230   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           41        41           
  Lines         1307      1332   +25     
=========================================
+ Hits          1307      1332   +25

Impacted Files	Coverage Δ
openapi_python_client/parser/openapi.py	`100.00% <100.00%> (ø)`
openapi_python_client/parser/properties.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7a483dc...2a8c51f. Read the comment docs.

dbanty

A couple things:

I think we should reduce the complexity of model to_dict back down unless there's a good reason for it to be so configurable.
I think we need to handle the type Unset everywhere it's usable, otherwise we're lying about the type which could lead to exceptions from client consumers.

dbanty · 2020-11-03T15:16:57Z

end_to_end_tests/golden-record/my_test_api_client/api/tests/defaults_tests_defaults_post.py

+    date_prop: datetime.date = isoparse("1010-10-10").date(),
+    float_prop: float = 3.14,
+    int_prop: int = 7,
+    boolean_prop: bool = cast(bool, UNSET),


I think the right thing to do with these would be to make the type Union[bool, Unset] when the prop is not required. That way we're not lying about the data that could be in an instance of this class.

dbanty · 2020-11-03T15:18:31Z

end_to_end_tests/golden-record/my_test_api_client/types.py


 import attr

+Unset = NewType("Unset", object)
+UNSET: Any = Unset(object())


Is the type of this not Unset? I haven't actually used NewType before.

I would say if we can't use Unset as an actual type both here and when typing potential UNSETs, we should class Unset: to make it possible.

dbanty · 2020-11-03T15:23:49Z

openapi_python_client/templates/model.pyi

+        include: Optional[Set[str]] = None,
+        exclude: Optional[Set[str]] = None,
+        exclude_unset: bool = False,
+        exclude_none: bool = False,


A couple notes here:

Are these here just in case someone wants them? Or was there a request specifically for this? I'd rather not add stuff to the API unless there's a demand for it since it slows things down a bit and might mean we have to breaking change it in the future.

When would we ever want to not exclude_unset?

My instinct is to scratch these params and always skip values that are UNSET.

The need originally spawned out of #228, but realistically theres no need atm as the separation between required and nullable fixes the issue I was trying to address

openapi_python_client/__init__.py

openapi_python_client/parser/properties.py

Co-authored-by: Dylan Anthony <43723790+dbanty@users.noreply.github.com>

emann · 2020-11-03T17:59:11Z

end_to_end_tests/golden-record/my_test_api_client/models/a_model.py

+
+        if self.nested_list_of_enums is UNSET:
+            nested_list_of_enums = UNSET
        else:
            nested_list_of_enums = []
            for nested_list_of_enums_item_data in self.nested_list_of_enums:


@dbanty I'm having a lot of trouble with blocks like this - mypy doesn't like "reassigning" in the else:
Incompatible types in assignment (expression has type "List[<nothing>]", variable has type "Unset")
What I could do is add a nested_list_of_enums: Union[Unset, List[List[DifferentEnum]]] = UNSET before the if statement and change the if/else to just be if self.nested_list_of_enums is not None

Mypy also complains about line 44 as it thinks the type of self.nested_list_of_enums can still be Unset. Changing line 40 to be if isinstance(self.nested_list_of_enums, Unset) fixes this issue, giving way to a new pattern combining both:

nested_list_of_enums: Union[Unset, List[List[DifferentEnum]]] = UNSET if not isinstance(self.nested_list_of_enums, Unset): nested_list_of_enums = [] etc.

The only problem is this gets pretty icky when trying to implement in the templates + account for stuff that can be None. Any advice on how to do this in a way thats less icky to implement?

For the if/else thing, you can declare the type before the block like:

nested_list_of_enums: Union[Unset, List[List[DifferentEnum]]] if self.nested_list_of_enums is UNSET: nested_list_of_enums = UNSET else: nested_list_of_enums = []

I believe mypy will be fine with that, though I'm not sure it's much better than what you had. Unfortunately I think is instance is the only way to handle the branches, because we told mypy it can be any Unset, not just UNSET. I don't think you can use Literal with an object...

Maybe we could do what Pydantic does and use ellipsis ... for our unset placeholder?

thing: Union[int, Literal[...]] = ... if thing is ...: ... else: ...

I don't know how you or mypy would feel about that. We could also alias UNSET = Literal[...] for clearer typing.

emann · 2020-11-04T18:32:22Z

@dbanty Ready for another review pass - unfortunately mypy didn't like Literal[...] (even if I did), so I went with something similar to what I had mentioned which turned out to not be too gross to implement.

dbanty

A couple nit pickles, mostly related to combining no_optional and no_unset because I think we don't need both. If you disagree with me that's fine, just leave them as is.

Also please merge main into this

dbanty · 2020-11-04T21:32:25Z

end_to_end_tests/golden-record/my_test_api_client/api/tests/defaults_tests_defaults_post.py

        json_list_prop = []
        for list_prop_item_data in list_prop:
            list_prop_item = list_prop_item_data.value

            json_list_prop.append(list_prop_item)

-    if union_prop is None:
-        json_union_prop: Optional[Union[Optional[float], Optional[str]]] = None
+    json_union_prop: Union[Unset, Union[float, str]]


Would be nice to handle nested unions better. I wonder if there's a function in typing somewhere that can simplify types like this. Not necessary for this PR but would be a good future enhancement.

I think manually adding Unset to the union should work fine, I'll give that a shot

dbanty · 2020-11-04T21:33:32Z

end_to_end_tests/golden-record/my_test_api_client/api/tests/defaults_tests_defaults_post.py

+    date_prop: Union[Unset, datetime.date] = isoparse("1010-10-10").date(),
+    float_prop: Union[Unset, float] = 3.14,
+    int_prop: Union[Unset, int] = 7,
+    boolean_prop: Union[Unset, bool] = UNSET,


This default used to be False, was that a mistake or did we change the spec?

if self.default fails to catch the default of False... oopsie

openapi_python_client/parser/properties.py

dbanty · 2020-11-04T21:43:51Z

openapi_python_client/parser/properties.py

+        elif self.nullable:
            default = "None"


Maybe we should leave it for now to not mess up existing clients, but now that we have UNSET, I don't think we want to default to None. The only reason that was the behavior was because it took the place of UNSET, but if None is a legitimate value that will get passed up to the API, I think it should have to be specified.

What do you think?

openapi_python_client/parser/properties.py

openapi_python_client/templates/model.pyi

openapi_python_client/templates/property_templates/union_property.pyi

dbanty · 2020-11-04T22:04:44Z

pyproject.toml

@@ -55,7 +55,7 @@ isort .\
 && flake8 openapi_python_client\
 && safety check --bare\
 && mypy openapi_python_client\
- && pytest --cov openapi_python_client tests\
+ && pytest --cov openapi_python_client tests --cov-report=term-missing\


What's that do?

When it shows the per-file coverage percentages in the report, it also shows which line numbers are missing next to it which is an absolute godsend

Co-authored-by: Dylan Anthony <43723790+dbanty@users.noreply.github.com>

…d but nullable properties no longer have a default value

into feature/add-unset-value-and-model-to-dict-params

emann · 2020-11-06T14:54:40Z

@dbanty Main merged, bool default fixed, required but nullable defaults removed, and non-required Union type strings are less ugly now - should be good to go (hopefully lol)

dbanty

I found another bug, sorry 😰

end_to_end_tests/golden-record/my_test_api_client/models/a_model.py

dbanty

Approved! 🎉 Squashing onto main, will make it into 0.7.0 after I finish #236 .

bowenwr · 2020-11-13T23:24:51Z

Thank you so much for this @emannguitar and @dbanty! Sorry for not responding, I was on the road and not checking notifications.

emann added 4 commits November 2, 2020 13:50

Added support for UNSET values and better differentiation between req…

be09dad

…uired and nullable. Model to_dict methods now have parameters to alter what fields are included/excluded

Fixed typing issues in golden record and added some test endpoints

921f235

Brought test coverage back to what it was before

ae9e2b3

Updated Changelog

957ce46

emann mentioned this pull request Nov 2, 2020

Remove optional generated class attributes set to None from serialized payload #205

Closed

dbanty requested changes Nov 3, 2020

View reviewed changes

emann and others added 2 commits November 3, 2020 11:39

Apply suggestions from code review

fb4cb6f

Co-authored-by: Dylan Anthony <43723790+dbanty@users.noreply.github.com>

Removed to_dict params and cleaned up type strings

8f7d51b

emann commented Nov 3, 2020

View reviewed changes

dbanty added this to the 0.7.0 milestone Nov 4, 2020

Fixed typing issues (again)

961f6df

dbanty requested changes Nov 4, 2020

View reviewed changes

emann and others added 3 commits November 6, 2020 09:23

Apply suggestions from code review

0ab3ddf

Co-authored-by: Dylan Anthony <43723790+dbanty@users.noreply.github.com>

Cleaned up union type strings when property is not required + require…

9e9ebbe

…d but nullable properties no longer have a default value

Merge branch 'main' of https://github.com/triaxtec/openapi-python-client

6bc9542

into feature/add-unset-value-and-model-to-dict-params

Regenerated the golden record

a669ba6

dbanty requested changes Nov 6, 2020

View reviewed changes

end_to_end_tests/golden-record/my_test_api_client/models/a_model.py Show resolved Hide resolved

end_to_end_tests/golden-record/my_test_api_client/models/a_model.py Show resolved Hide resolved

emann added 2 commits November 6, 2020 11:01

UNSET is now a falsey value

65cd367

Fixed changelog entry

9926753

dbanty mentioned this pull request Nov 6, 2020

Support inline object schemas #236

Merged

8 tasks

dbanty approved these changes Nov 6, 2020

View reviewed changes

Regenerated e2e tests

2a8c51f

dbanty merged commit 296ffb6 into openapi-generators:main Nov 6, 2020

dbanty mentioned this pull request Nov 6, 2020

Add ability to omit certain fields from a model when converting to a dict #228

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better compatibility for "required" vs. "nullable" #230

Better compatibility for "required" vs. "nullable" #230

emann commented Nov 2, 2020

codecov bot commented Nov 2, 2020 •

edited

Loading

dbanty left a comment

dbanty Nov 3, 2020

dbanty Nov 3, 2020

dbanty Nov 3, 2020

dbanty Nov 3, 2020

emann Nov 3, 2020

emann Nov 3, 2020

dbanty Nov 3, 2020

emann commented Nov 4, 2020

dbanty left a comment

dbanty Nov 4, 2020

emann Nov 6, 2020

dbanty Nov 4, 2020

emann Nov 6, 2020

dbanty Nov 4, 2020

dbanty Nov 4, 2020

emann Nov 6, 2020

emann commented Nov 6, 2020

dbanty left a comment

dbanty left a comment

bowenwr commented Nov 13, 2020

Better compatibility for "required" vs. "nullable" #230

Better compatibility for "required" vs. "nullable" #230

Conversation

emann commented Nov 2, 2020

codecov bot commented Nov 2, 2020 • edited Loading

Codecov Report

dbanty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emann commented Nov 4, 2020

dbanty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emann commented Nov 6, 2020

dbanty left a comment

Choose a reason for hiding this comment

dbanty left a comment

Choose a reason for hiding this comment

bowenwr commented Nov 13, 2020

codecov bot commented Nov 2, 2020 •

edited

Loading