Skip to content

Commit 9dcce0c

Browse files
committed
Code review feedback - preserve Optional typing and change defaults
1 parent 262022e commit 9dcce0c

File tree

2 files changed

+40
-110
lines changed

2 files changed

+40
-110
lines changed

openapi_python_client/parser/properties.py

Lines changed: 21 additions & 72 deletions
Original file line numberDiff line numberDiff line change
@@ -51,16 +51,9 @@ def get_type_string(self, no_optional: bool = False) -> str:
5151
Args:
5252
no_optional: Do not include Optional even if the value is optional (needed for isinstance checks)
5353
"""
54-
return type_string(no_optional, self, self._type_string)
55-
56-
def get_query_method_arg(self, no_optional: bool = False) -> str:
57-
"""
58-
Get a string representation of type that should be used when the property is represented in a query string.
59-
60-
Args:
61-
no_optional: Do not include Optional even if the value is optional (needed for isinstance checks)
62-
"""
63-
return query_method_arg(no_optional, self, self._type_string)
54+
if no_optional or (self.required and not self.nullable):
55+
return self._type_string
56+
return f"Optional[{self._type_string}]"
6457

6558
def get_imports(self, *, prefix: str) -> Set[str]:
6659
"""
@@ -70,11 +63,9 @@ def get_imports(self, *, prefix: str) -> Set[str]:
7063
prefix: A prefix to put before any relative (local) module names. This should be the number of . to get
7164
back to the root of the generated client.
7265
"""
73-
if self.required and self.nullable:
74-
return {"from typing import Optional"}
75-
elif not self.required:
76-
return {"from typing import Union",
77-
f"from {prefix}types import Unset",
66+
if self.nullable or not self.required:
67+
return {"from typing import Optional",
68+
"from typing import cast",
7869
f"from {prefix}types import UNSET"}
7970
return set()
8071

@@ -83,7 +74,7 @@ def to_string(self) -> str:
8374
if self.default:
8475
default = self.default
8576
elif not self.required:
86-
default = "UNSET"
77+
default = "cast(None, UNSET)"
8778
else:
8879
default = None
8980

@@ -102,10 +93,9 @@ def to_query_method_arg(self) -> str:
10293
default = None
10394

10495
if default is not None:
105-
return f"{self.python_name}: {self.get_query_method_arg()} = {default}"
96+
return f"{self.python_name}: {self.get_type_string()} = {default}"
10697
else:
107-
return f"{self.python_name}: {self.get_query_method_arg()}"
108-
98+
return f"{self.python_name}: {self.get_type_string()}"
10999

110100
@dataclass
111101
class StringProperty(Property):
@@ -247,13 +237,9 @@ class ListProperty(Property, Generic[InnerProp]):
247237

248238
def get_type_string(self, no_optional: bool = False) -> str:
249239
""" Get a string representation of type that should be used when declaring this property """
250-
return type_string(no_optional, self, f"List[{self.inner_property.get_type_string()}]")
251-
252-
def get_query_method_arg(self, no_optional: bool = False) -> str:
253-
"""
254-
Get a string representation of type that should be used when the property is represented in a query string.
255-
"""
256-
return query_method_arg(no_optional, self, f"List[{self.inner_property.get_query_method_arg()}]")
240+
if no_optional or (self.required and not self.nullable):
241+
return f"List[{self.inner_property.get_type_string()}]"
242+
return f"Optional[List[{self.inner_property.get_type_string()}]]"
257243

258244
def get_imports(self, *, prefix: str) -> Set[str]:
259245
"""
@@ -283,15 +269,9 @@ def get_type_string(self, no_optional: bool = False) -> str:
283269
""" Get a string representation of type that should be used when declaring this property """
284270
inner_types = [p.get_type_string() for p in self.inner_properties]
285271
inner_prop_string = ", ".join(inner_types)
286-
return type_string(no_optional, self, inner_prop_string, is_outer_union=True)
287-
288-
def get_query_method_arg(self, no_optional: bool = False) -> str:
289-
"""
290-
Get a string representation of type that should be used when the property is represented in a query string.
291-
"""
292-
inner_types = [p.get_query_method_arg() for p in self.inner_properties]
293-
inner_prop_string = ", ".join(inner_types)
294-
return query_method_arg(no_optional, self, inner_prop_string, is_outer_union=True)
272+
if no_optional or (self.required and not self.nullable):
273+
return f"Union[{inner_prop_string}]"
274+
return f"Optional[Union[{inner_prop_string}]]"
295275

296276
def get_imports(self, *, prefix: str) -> Set[str]:
297277
"""
@@ -363,13 +343,10 @@ def get_enum(name: str) -> Optional["EnumProperty"]:
363343

364344
def get_type_string(self, no_optional: bool = False) -> str:
365345
""" Get a string representation of type that should be used when declaring this property """
366-
return type_string(no_optional, self, self.reference.class_name)
367346

368-
def get_query_method_arg(self, no_optional: bool = False) -> str:
369-
"""
370-
Get a string representation of type that should be used when the property is represented in a query string.
371-
"""
372-
return query_method_arg(no_optional, self, self.reference.class_name)
347+
if no_optional or (self.required and not self.nullable):
348+
return self.reference.class_name
349+
return f"Optional[{self.reference.class_name}]"
373350

374351
def get_imports(self, *, prefix: str) -> Set[str]:
375352
"""
@@ -428,13 +405,9 @@ def template(self) -> str: # type: ignore
428405

429406
def get_type_string(self, no_optional: bool = False) -> str:
430407
""" Get a string representation of type that should be used when declaring this property """
431-
return type_string(no_optional, self, self.reference.class_name)
432-
433-
def get_query_method_arg(self, no_optional: bool = False) -> str:
434-
"""
435-
Get a string representation of type that should be used when the property is represented in a query string.
436-
"""
437-
return query_method_arg(no_optional, self, self.reference.class_name)
408+
if no_optional or (self.required and not self.nullable):
409+
return self.reference.class_name
410+
return f"Optional[{self.reference.class_name}]"
438411

439412
def get_imports(self, *, prefix: str) -> Set[str]:
440413
"""
@@ -617,27 +590,3 @@ def property_from_data(
617590
return _property_from_data(name=name, required=required, data=data)
618591
except ValidationError:
619592
return PropertyError(detail="Failed to validate default value", data=data)
620-
621-
622-
def type_string(no_optional: bool, prop: Property, inner_type_string: str, is_outer_union: bool = False) -> str:
623-
if no_optional or (prop.required and not prop.nullable):
624-
if is_outer_union:
625-
return f"Union[{inner_type_string}]"
626-
return inner_type_string
627-
elif prop.nullable and prop.required:
628-
if is_outer_union:
629-
return f"Optional[Union[{inner_type_string}]]"
630-
return f"Optional[{inner_type_string}]"
631-
elif not prop.nullable and not prop.required:
632-
return f"Union[Unset, None, {inner_type_string}]"
633-
return f"Union[Unset, {inner_type_string}]"
634-
635-
636-
def query_method_arg(no_optional: bool, prop: Property, inner_type_string: str, is_outer_union: bool = False) -> str:
637-
if no_optional or (prop.required and not prop.nullable):
638-
if is_outer_union:
639-
return f"Union[{inner_type_string}]"
640-
return inner_type_string
641-
if is_outer_union:
642-
return f"Optional[Union[{inner_type_string}]]"
643-
return f"Optional[{inner_type_string}]"

tests/test_openapi_parser/test_properties.py

Lines changed: 19 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -24,23 +24,13 @@ def test_get_type_string(self):
2424
p = Property(name="test", required=True, default=None, nullable=False)
2525
p._type_string = "TestType"
2626

27-
# Nullable = False, Required = True
2827
assert p.get_type_string() == "TestType"
29-
30-
# Nullable = False, Required = False, No Default
31-
p.nullable = False
3228
p.required = False
33-
assert p.get_type_string() == "Union[Unset, None, TestType]"
29+
assert p.get_type_string() == "Optional[TestType]"
3430
assert p.get_type_string(True) == "TestType"
3531

36-
# Nullable = True, Required = False, No Default
3732
p.required = False
3833
p.nullable = True
39-
assert p.get_type_string() == "Union[Unset, TestType]"
40-
41-
# Nullable = True, Required = True, No Default
42-
p.required = True
43-
p.nullable = True
4434
assert p.get_type_string() == "Optional[TestType]"
4535

4636
def test_to_string(self, mocker):
@@ -52,50 +42,41 @@ def test_to_string(self, mocker):
5242
get_type_string = mocker.patch.object(p, "get_type_string")
5343

5444
assert p.to_string() == f"{snake_case(name)}: {get_type_string()}"
55-
p.required = True
56-
p.nullable = True
57-
assert p.to_string() == f"{snake_case(name)}: {get_type_string()}"
45+
p.required = False
46+
assert p.to_string() == f"{snake_case(name)}: {get_type_string()} = cast(None, UNSET)"
5847

5948
p.default = "TEST"
6049
assert p.to_string() == f"{snake_case(name)}: {get_type_string()} = TEST"
6150

62-
p.nullable = False
63-
p.required = False
64-
p.default = None
65-
assert p.to_string() == f"{snake_case(name)}: {get_type_string()} = UNSET"
66-
67-
def test_to_query_method_arg(self, mocker):
51+
def test_get_imports(self, mocker):
6852
from openapi_python_client.parser.properties import Property
6953

7054
name = mocker.MagicMock()
71-
snake_case = mocker.patch("openapi_python_client.utils.snake_case")
55+
mocker.patch("openapi_python_client.utils.snake_case")
7256
p = Property(name=name, required=True, default=None, nullable=False)
73-
get_query_string = mocker.patch.object(p, "get_query_method_arg")
57+
assert p.get_imports(prefix="") == set()
7458

75-
assert p.to_query_method_arg() == f"{snake_case(name)}: {get_query_string()}"
7659
p.required = False
77-
assert p.to_query_method_arg() == f"{snake_case(name)}: {get_query_string()} = None"
78-
79-
p.default = "TEST"
80-
assert p.to_query_method_arg() == f"{snake_case(name)}: {get_query_string()} = TEST"
60+
assert p.get_imports(prefix="") == {
61+
"from types import UNSET",
62+
"from typing import Optional",
63+
"from typing import cast",
64+
}
8165

82-
def test_get_imports(self, mocker):
66+
def test_to_query_method_arg(self, mocker):
8367
from openapi_python_client.parser.properties import Property
8468

8569
name = mocker.MagicMock()
86-
mocker.patch("openapi_python_client.utils.snake_case")
70+
snake_case = mocker.patch("openapi_python_client.utils.snake_case")
8771
p = Property(name=name, required=True, default=None, nullable=False)
88-
assert p.get_imports(prefix="") == set()
89-
90-
p.nullable = True
91-
assert p.get_imports(prefix="") == {"from typing import Optional"}
72+
get_type_string = mocker.patch.object(p, "get_type_string")
9273

74+
assert p.to_query_method_arg() == f"{snake_case(name)}: {get_type_string()}"
9375
p.required = False
94-
assert p.get_imports(prefix="..") == {
95-
"from typing import Union",
96-
"from ..types import Unset",
97-
"from ..types import UNSET",
98-
}
76+
assert p.to_query_method_arg() == f"{snake_case(name)}: {get_type_string()} = None"
77+
78+
p.default = "TEST"
79+
assert p.to_query_method_arg() == f"{snake_case(name)}: {get_type_string()} = TEST"
9980

10081
def test__validate_default(self):
10182
from openapi_python_client.parser.properties import Property

0 commit comments

Comments
 (0)