Fixed get_component method in Field to get working with subclassess of collections.Mapping #2140
Conversation
| hasattr(obj, '__getitem__') and | ||
| hasattr(obj, '__len__') and | ||
| hasattr(obj, '__iter__') and | ||
| hasattr(obj, 'get')): |
There was a problem hiding this comment.
Not super-keen on this. Can we use isinstance(obj, (dict, Mapping)) instead? Any other cleaner options?
|
Would be happy to support |
|
I've just updated pr with this check: isinstance(obj, (dict, Mapping)). |
| def __iter__(self): | ||
| return iter(self._data) | ||
|
|
||
|
|
There was a problem hiding this comment.
Given that this is just a test case, can we stub this out to just __getitem__?
And to also just do self._data = data, ignoring the None case?
Those codepaths won't ever be run, so be better to drop them.
Let's also include a docstring explaining the motivation of the class.
There was a problem hiding this comment.
Abstract base class Mapping forces to define all these methods - __getitem__, __len__, __iter__.
Yes, we can ignore None case, i'll update pr shortly.
|
You know what? Let's just drop the test case. Contrary to popular opinion code that makes sense is actually sufficient, we don't need to test every little edge case and the 'isinstance' check is obvious and correct. Let's just have the code change for this. |
| @@ -52,7 +53,7 @@ def get_component(obj, attr_name): | |||
| Given an object, and an attribute name, | |||
| return that attribute on the object. | |||
| """ | |||
| if isinstance(obj, dict): | |||
| if isinstance(obj, (dict, collections.Mapping)): | |||
There was a problem hiding this comment.
According to the documentation, dict is a subclass of Mapping, so isinstance(obj, collections.Mapping) should be sufficient.
There was a problem hiding this comment.
Yup, and confirmed this is true all the way down to 2.6
|
Just noticed that this one is for 2.x. |
…f collections.Mapping
|
Ok, so i've updated pull request according to proposed changes. |
Fixed get_component method in Field to get working with subclassess of collections.Mapping
Method get_components checks if object is instance of dictionary, but it fails when custom dict-alike implementations (especially subclasses of collections.Mapping) are used.
This commit adds checking for presence of dictionary interface methods on obj in method get_components.