Skip to content

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 21, 2021
Merged

fix(types): prevent __dir__ in RestObject from producing duplicates #1383

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5bf7525
fix(types): prevent __dir__ from producing duplicates
spyoungtech Mar 23, 2021
a8e591f
test(object): add test for __dir__ duplicates
spyoungtech Apr 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion gitlab/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Copy link
Member

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())

Copy link
Contributor Author

@spyoungtech spyoungtech Apr 21, 2021 •
edited
Loading

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.

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.

Suggested change
return super(RESTObject, self).__dir__() | self.attributes.keys()
return list(super(RESTObject, self).__dir__() | self.attributes.keys())

Copy link
Member

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.

Copy link
Member

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 of Iterable[something], most likely.

Copy link
Contributor Author

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'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.


def __hash__(self) -> int:
if not self.get_id():
Expand Down
4 changes: 4 additions & 0 deletions gitlab/tests/test_base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,10 @@ def test_update_attrs_deleted(self, fake_manager):
assert {"foo": "foo"} == obj._attrs
assert {} == obj._updated_attrs

def test_dir_unique(self, fake_manager):
obj = FakeObject(fake_manager, {"manager": "foo"})
assert len(dir(obj)) == len(set(dir(obj)))

def test_create_managers(self, fake_gitlab, fake_manager):
class ObjectWithManager(FakeObject):
_managers = (("fakes", "FakeManager"),)
Expand Down