fix(types): prevent __dir__ in RestObject from producing duplicates #1383
Conversation
|
@JohnVillalovos I think this would work, because dict |
Ah correct. I thought both were lists. |
| @@ -132,7 +132,7 @@ def __ne__(self, other: object) -> bool: | |||
| return super(RESTObject, self) != other | |||
|
|
|||
| def __dir__(self): | |||
| return super(RESTObject, self).__dir__() + list(self.attributes) | |||
| return super(RESTObject, self).__dir__() | self.attributes.keys() | |||
There was a problem hiding this comment.
Can the result be converted to a list? As that is what it returned before.
return list(super(RESTObject, self).__dir__() | self.attributes.keys())
There was a problem hiding this comment.
The __dir__ method can return any sequence and will be converted into a list and sorted automatically. For example, if you return a set or any other sequence, dir(obj) will always be a list.
class MyObj:
def __dir__(self):
return {'abc', 'xyz', 'def',}
obj = MyObj()
dir(obj)
['abc', 'def', 'xyz']If you like we can convert it in the method directly, if for nothing else, to be more explicit, though the result is the same in either case.
| return super(RESTObject, self).__dir__() | self.attributes.keys() | |
| return list(super(RESTObject, self).__dir__() | self.attributes.keys()) |
There was a problem hiding this comment.
I (for selfish reasons) would like it to return a list. As I am going to be working on adding type-hints to that code in the future 😊 But not necessary.
There was a problem hiding this comment.
And not necessary, because I think I will be adding a type-hint for the return value of Iterable[something], most likely.
There was a problem hiding this comment.
Sounds good to me. Feel free to apply the above suggestion (or not)
Looking forward to type hinting!
Worth mentioning that it's usually not necessary to provide type annotations or type hints for certain magic methods, since their return type is implied because there's no way to implement them to return a different type.
For example, the return type for methods like __str__, __repr__, __len__, __dir__, are known to IDEs and type checkers like MyPy automatically because the returned type must be valid, otherwise a TypeError is raised.
|
Went ahead and added test case @nejch Confirmed it fails on the main branch, but passes with the changes this PR adds. |
Great, thanks @spyoungtech! |
Fixes a small issue with the feature added in #1072 for
__dir__to display rest object attributes.In at least one case, classes have overlapping attributes with the attributes listed in
.attributes. For example, theGroupobject has its ownprojectsproperty for theGroupProjectManager. This shadows the name that would normally access.attributes['projects']. This causesdir(some_group), as currently implemented, to listprojectstwice.While this is unlikely to be problematic, it's ideal if duplicate attributes do not appear in the response for
__dir__. This PR ensures that duplicates are not produced by the__dir__method.Instead of adding the list of attributes, we use the
|operator to produce a set with the union of all the elements, ensuring there are no duplicates.