Implemented easydict. #1094
Implemented easydict. #1094
Conversation
|
|
|
This PR needs Approvals as follows.
Please choose reviewers and requet reviews! Click to see how to approve each reviewsYou can approve this PR by triggered comments as follows.
See all trigger commentsPlease replace [Target] to review target
|
| @@ -0,0 +1,30 @@ | |||
| class SmartDict(dict): | |||
| @@ -0,0 +1,63 @@ | |||
| import pytest | |||
|
@tfujiwar Please review 🙇 |
|
I'll check tomorrow so please wait for a while. Thanks! |
| super(SmartDict, self).__init__() | ||
| self.update(d or {}, **kwargs) | ||
|
|
||
| def update(self, d=None, **kwargs): |
There was a problem hiding this comment.
Can we make this method non-public? _update
There was a problem hiding this comment.
@a-hanamoto
Since the update function is in dict, I think it is necessary for SmartDict which inherits dict.
https://docs.python.org/ja/3/library/stdtypes.html?highlight=dict#dict.update
There was a problem hiding this comment.
Then could you please write test cases for update?
There was a problem hiding this comment.
I added test case for update function. Please review it.
blueoil/utils/smartdict.py
Outdated
| class SmartDict(dict): | ||
| def __init__(self, d=None, **kwargs): | ||
| super(SmartDict, self).__init__() | ||
| self.update(d or {}, **kwargs) |
There was a problem hiding this comment.
d will be replaced d or {} soon after calling update, so d is OK, I think.
| def __setitem__(self, name, value): | ||
| if isinstance(value, (list, tuple)): | ||
| value = [ | ||
| self.__class__(x) if isinstance(x, dict) else x |
There was a problem hiding this comment.
SmartDict? I wonder why there are both of self.__class__ and SmartDict
There was a problem hiding this comment.
@a-hanamoto
Your question is, why do you write "self.class(x)" instead of "SmartDict(x)"?
This is because if you inherit "SmartDict", "self.class" will be a subclass.
class HyperSmartDict(SmartDict):
pass
d = HyperSmartDict()
d.__class__ == HyperSmartDict
| super(SmartDict, self).__init__() | ||
| self.update(d or {}, **kwargs) | ||
|
|
||
| def update(self, d=None, **kwargs): |
There was a problem hiding this comment.
Then could you please write test cases for update?
| def __setitem__(self, name, value): | ||
| if isinstance(value, (list, tuple)): | ||
| value = [ | ||
| self.__class__(x) if isinstance(x, dict) else x |
blueoil/utils/smartdict.py
Outdated
| def __getattr__(self, name): | ||
| if name in self: | ||
| return self[name] | ||
| return super(SmartDict, self).__getattr__(name) |
There was a problem hiding this comment.
I think __getattr__ should raise an AttributeError instead of delegating to the super class. Because Dict doesn't have a method __getattr__, the current error message is like:
AttributeError: 'super' object has no attribute '__getattr__'
It should be like:
AttributeError: 'SmartDict' object has no attribute 'x'
blueoil/utils/smartdict.py
Outdated
| if hasattr(self, name): | ||
| return super(SmartDict, self).__setattr__(name, value) | ||
| self[name] = value |
There was a problem hiding this comment.
I think self[name] = value is enough. Currently, the results are different for an existing and non-existing key.
d = SmartDict(a=None)
d.a = [{"k":"v"}]
d.b = [{"k":"v"}]
d.a[0].__class__ # dict
d.b[0].__class__ # SmartDict
|
/ready |
|
⏳Merge job is queued... |
|
😥This PR is not approved yet. |
|
OA |
|
/ready |
|
⏳Merge job is queued... |
What this patch does to fix the issue.
#1092