Fully translate nested objects in API responses #306
Conversation
|
Verified that @mattwiller has signed the CLA. Thanks for the pull request! |
|
Is this a WIP, or final code for review? |
|
@jmoldow This is more like a first draft — this works in my local testing, so it would be great if you could look it over and evaluate if you think this is the right approach! |
There was a problem hiding this comment.
My initial thoughts are that this is a rather expensive method. It will create a lot of objects that very often will be totally unused. I wonder if there's a lazy way to do this. For example, we could wait until an object is accessed via __getitem__ to create the smart object.
boxsdk/util/translator.py
Outdated
|
|
||
| if isinstance(response_object, Mapping): | ||
| for key in response_object: | ||
| if isinstance(response_object[key], Mapping): |
There was a problem hiding this comment.
I think you can just use dict instead of Mapping since that's what the JSON parser creates.
boxsdk/util/translator.py
Outdated
| for i in range(len(response_object[key])): | ||
| response_object[key][i] = self.full_translate(response_object[key][i], session) | ||
|
|
||
| if 'type' in response_object: |
There was a problem hiding this comment.
is type totally reserved in the Box API? Can you create a metadata template with type as one of the keys? If so, the code needs to take this into account.
boxsdk/util/translator.py
Outdated
| if isinstance(response_object[key], Mapping): | ||
| response_object[key] = self.full_translate(response_object[key], session) | ||
| elif isinstance(response_object[key], list): | ||
| for i in range(len(response_object[key])): |
There was a problem hiding this comment.
the pythonic way would be to use a list comprehension like
response_object[key] = [self.full_translate(o, session) for o in response_object[key]]
boxsdk/util/translator.py
Outdated
| return response_object | ||
|
|
||
| def _is_constructor_param(self, param): | ||
| if param.name is 'self': |
There was a problem hiding this comment.
compare with == not is
mattwiller
force-pushed
the
nested_object_translation
branch
from
3f99a06
to
0419960
Compare
boxsdk/util/translator.py
Outdated
| 'response_object': response_object, | ||
| 'object_id': _get_object_id(response_object), | ||
| } | ||
| # NOTE: getargspec() is deprecated, and should be replaced by inspect.signature() when 2.7 support drops |
There was a problem hiding this comment.
I hope 2.7 support doesn't outlast support for getargspec, but it might. It could be safer or more correct to use inspect.signature, falling back to getargspec when unavailable.
I think the mock library does something similar.
|
@Jeff-Meadows It looks like Also, your previous comment about this method being more expensive is correct — it takes about 45x longer to fully translate than to shallow translate in the tests I ran (against a collection of 1000 files in a folder, as well as a single file). I'm not sure how dire that is in context, but the difference I observed (a few hundred microseconds) for each one of a few thousand items in an iterator could certainly add up to non-trivial amounts of time. I'll also explore how easy it would be to lazily translate nested objects; the case I'm currently worried about is when an |
Pull Request Test Coverage Report for Build 1202
💛 - Coveralls |
|
@mattwiller one more comment - we should review carefully and perhaps give some suggestions to SDK users about when to use |
No description provided.