- Notifications
You must be signed in to change notification settings - Fork674
fix(types): prevent __dir__ in RestObject from producing duplicates#1383
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
JohnVillalovos left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think this needs a unit test.
In my quick testing of this idea it doesn't seem to work.
>>> a = [1,2] | [3,4]Traceback (most recent call last): File "<stdin>", line 1, in <module>TypeError: unsupported operand type(s) for |: 'list' and 'list'nejch commentedApr 20, 2021
@JohnVillalovos I think this would work, because dict |
JohnVillalovos commentedApr 20, 2021
Ah correct. I thought both were lists. |
| def__dir__(self): | ||
| returnsuper(RESTObject,self).__dir__()+list(self.attributes) | ||
| returnsuper(RESTObject,self).__dir__()|self.attributes.keys() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can the result be converted to a list? As that is what it returned before.
return list(super(RESTObject, self).__dir__() | self.attributes.keys())
spyoungtechApr 21, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
classMyObj: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.
| returnsuper(RESTObject,self).__dir__()|self.attributes.keys() | |
| returnlist(super(RESTObject,self).__dir__()|self.attributes.keys()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
And not necessary, because I think I will be adding a type-hint for the return value ofIterable[something], most likely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sounds good to me. Feel free to apply the above suggestion (or not)
Looking forward to type hinting!
Worth mentioning that it'susually 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 aTypeError is raised.
spyoungtech commentedApr 21, 2021
Went ahead and added test case@nejch Confirmed it fails on the main branch, but passes with the changes this PR adds. |
nejch commentedApr 21, 2021
Great, thanks@spyoungtech! |
Uh oh!
There was an error while loading.Please reload this page.
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 ofadding the list of attributes, we use the
|operator to produce a set with the union of all the elements, ensuring there are no duplicates.