In at least one case, classes have overlapping attributes with the attributes listed in.attributes. For example, theGroup object has its ownprojects property for theGroupProjectManager. This shadows the name that would normally access.attributes['projects']. This causesdir(some_group), as currently implemented, to listprojects twice.

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.

fix(types): prevent __dir__ from producing duplicates

5bf7525

JohnVillalovos requested changes

Apr 20, 2021

View reviewed changes

Copy link

Member

JohnVillalovos left a 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'

Copy link

Member

nejch commentedApr 20, 2021

@JohnVillalovos I think this would work, because dictkeys() returns a set-like objecthttps://docs.python.org/3/library/stdtypes.html#dictionary-view-objects.
Although true, if you like@spyoungtech, maybe a quick test against the original issue would be great :)

Copy link

Member

JohnVillalovos commentedApr 20, 2021

@JohnVillalovos I think this would work, because dictkeys() returns a set-like objecthttps://docs.python.org/3/library/stdtypes.html#dictionary-view-objects.
Although true, if you like@spyoungtech, maybe a quick test against the original issue would be great :)

Ah correct. I thought both were lists.

JohnVillalovos reviewed

Apr 20, 2021

View reviewed changes

gitlab/base.py

		@@ -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

JohnVillalovosApr 20, 2021

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

ContributorAuthor

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

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.

Suggested change

	returnsuper(RESTObject,self).__dir__()\|self.attributes.keys()
	returnlist(super(RESTObject,self).__dir__()\|self.attributes.keys())

Copy link

Member

JohnVillalovosApr 21, 2021

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

JohnVillalovosApr 21, 2021

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.

Copy link

ContributorAuthor

spyoungtechApr 21, 2021

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.