Refactor BaseModel._deserialize#16
Conversation
hairmare
left a comment
hairmare
left a comment
There was a problem hiding this comment.
this looks like it reduces the complexity of _deserialize() method quite a bit!
one downside i see is that the new _get_included_item() is still tightly coupled to _deserialize(). this means it isn't easy to implement an isolated unit test for the new method.
| relationships = item.get("relationships") | ||
| if not relationships: | ||
|
|
||
| if not (relationships := item.get("relationships")): |
| for relationship_data in data | ||
| if ( | ||
| included_item := self._get_included_item( | ||
| relationship_data["type"], relationship_data["id"], included |
There was a problem hiding this comment.
is there a reason why it's not using relationship_data.get("type")
you seem to primarily use get() up until here
| relationships: list[tuple] | ||
| filters: list[tuple] | ||
|
|
||
| def _get_included_item(self, item_type: str, item_id, included: dict): |
There was a problem hiding this comment.
good job, this method doesn't contain a single walrus 
There was a problem hiding this comment.
That can be arranged
def _get_included_item(self, item_type: str, item_id, included: dict):
if not (included_item:= next(
(
included_item
for included_item in included
if included_item.get("type") == item_type
and included_item.get("id") == item_id
),
None,
)):
return None
included_model = self.client._type_map[item_type](self.client)
return included_model._deserialize(included_item, included)| if not included_item: | ||
| return None | ||
| included_model = self.client._type_map[item_type](self.client) | ||
| return included_model._deserialize(included_item, included) |
There was a problem hiding this comment.
_deserialize() used to fully contain the recursion logic, so the stack looked something like so:
_deserialize()_deserialize()_deserialize()- ...
with this change the stack will look more like this:
_deserialize()_get_included_item()_deserialize()_get_included_item()_deserialize()_get_included_item()- ...
is this a trade off, you're willing to make?
Alex Recommends ReportAlex recommends the following language changes, but Alex is a regular expression based algorithm, so take them with a grain of salt. ✨ 🚀 ✨ Nothing to Report ✨ 🚀 ✨ |
c0rydoras
force-pushed
the
refactor-deserialize
branch
from
bd9fe1d to
7586f6c
Compare
c0rydoras
force-pushed
the
refactor-deserialize
branch
from
7586f6c to
dd9f8de
Compare
2 participants
No description provided.