-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add equality rich comparision operators to telegram objects #604
Conversation
telegram/base.py
Outdated
raise NotImplementedError | ||
|
||
def __eq__(self, other): | ||
if isinstance(other, TelegramObject): |
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.
From the Photosize __eq__
it has:
if not isinstance(other, self.__class__):
Might use that here as if isinstance(other, self.__class__):
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.
Yes, good idea :D
Seems pretty solid. I added a comment.
Regarding tests, I would vote for adding them in their respective classes. |
@bomjacob @tsnoam For the tests :D see test_video.py line 159 and test_video.py line 159 |
Ahh, thanks, I see :D @Eldinnie I agree with implementing the other in their respective classes ^^ |
@bomjacob As far as i recall, telegram servers started to return actual data in reply to sending certain messages (sendPhoto from a url?) I believe that comparing id may be enough but I don't have insight into telegram internals... |
I want to point out an alternative to having the equality check for identity: One could also have two objects, I understand however that the use case for the equality operator might mostly be to check for identity, in which case the implementation based on |
That's not quite how python works though.... In [4]: user1 = telegram.User(1, "Same")
In [5]: user2 = telegram.User(2, "Same")
In [6]: user1 is user2
Out[6]: False |
But that's exactly what I'd want:
I'd expect:
The critical thing is that there shouldn't be the possibility of creating two objects Also, just to be explicit: I don't want to insist on this implementation. I just wanted to pitch this alternative and discuss it. It might be more powerful but I also suspect that this is not intuitive and too specialized. Also thanks for taking your time to implement it! :) |
Oh that was what you meant? |
@y0hy0h If you want to compare if users have the same first name, you could do that manually. What we want to achieve is to figure out if two What you suggest is like saying you live at privet drive nr 4. and when I come to visit I get to diagon ally nr 4 and expect you to be there. |
The |
Audio, Document, File, Location, Photo, Sticker, Venue, Video and Voice
@Eldinnie It's not about checking the name. It's more like @tsnoam That's a good point against enforcing users to be singletons. That also makes checking by My suggestion was just something that was intuitive to me. I simply feel that Also thank you for your patience and answers! It's really awesome that you addressed my issue in such detail! |
@y0hy0h |
ChosenInlineResult, InlineQuery, Message, Update and User
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.
See comments on the changes.
Basically, the comments about how to hash/eq/raise are relevant for all types.
telegram/base.py
Outdated
raise NotImplementedError | ||
|
||
def __eq__(self, other): | ||
if isinstance(other, self.__class__): |
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.
This is one of the few places where I prefer comparing the class, instead of using isinstance
. The reason is that multiple levels of inheritance may create undesirable effects.
So, I think if type(other) is type(self):
would be more appropriate here.
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.
As I see it, if I subclass User
as SpecialUser
for some reason I'd expect to be able to compare Users and SpecialUsers. The inheritence should only be able to go one way (other needs to be a subclass of self), so having them all subclass TelegramObject
is not an issue. Also I don't think we ever subclass a subclass of TelegramObject
anywhere in the library atm.
telegram/base.py
Outdated
for x in ['id', 'file_id', 'message_id', 'result_id', 'update_id']: | ||
if hasattr(self, x): | ||
return self.__getattribute__(x) | ||
raise NotImplementedError |
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 prefer RuntimeError
to be raised. After all, _get_id
is implemented...
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 return NotImplementedError as a way of saying "this object doesn't implement an id", which many objects do not (pretty much all objects not mentioned in the OP). This is why I'm catching it inside of __eq__
too.
telegram/base.py
Outdated
def _get_id(self): | ||
for x in ['id', 'file_id', 'message_id', 'result_id', 'update_id']: | ||
if hasattr(self, x): | ||
return self.__getattribute__(x) |
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 usually prefer using getattr(self, x)
. Any reason you chose to use __getattribute__
here?
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.
Any reason you chose to use
__getattribute__
here?
I'd forgotten it existed... I'll change it :D
telegram/base.py
Outdated
def __eq__(self, other): | ||
if isinstance(other, self.__class__): | ||
try: | ||
return self._get_id() == other._get_id() |
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.
Nitpick: you're accessing a private method _get_id()
of something other than self
.
Maybe it's the proper thing to do here, just wish that we think about it one more time.
Maybe return hash(self) == hash(other)
?
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'd say it's okay since I'm checking the type of the class first, so I know with almost 100% certainty how _get_id()
operates.
I seem to recall having read somewhere that basing __eq__
on hash is very discouraged.
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.
If it's a big issue then I can make the _get_id() works either as a static method or just move it out of the class entirely and then have it accept the object to extract id from.
telegram/base.py
Outdated
if isinstance(other, self.__class__): | ||
try: | ||
return self._get_id() == other._get_id() | ||
except NotImplementedError: |
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.
This shouldn't happen if you properly perform the class comparison above.
Maybe if we use hash
comparison will even save the need to compare the class all together.
telegram/base.py
Outdated
return self._get_id() == other._get_id() | ||
except NotImplementedError: | ||
pass # return NotImplemented | ||
return NotImplemented |
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.
Just return False
.
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 return NotImplemented as a way of telling the interpreter to try the comparison the other way around. This allows the other
to say "Hey! I know this object, and yes we are indeed equal".
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 can go with your take on isinstance
(above), but I don't think that returning something else than a bool
is right.
What about something simple like:
return isinstance(other, self.__class__) and self._get_id() == other._get_id()
telegram/base.py
Outdated
try: | ||
return hash(( | ||
self.__class__, | ||
self._get_id(),)) |
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.
Style nitpick: the entire return
statement can be in a single line...
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.
Not according to my yapf
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.
This is what I meant. Cleaner and works with yapf ;-)
def __hash__(self):
return hash((self.__class__, self._get_id()))
telegram/base.py
Outdated
return hash(( | ||
self.__class__, | ||
self._get_id(),)) | ||
except NotImplementedError: |
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 don't think catching NotIMplementedError
here is needed. If we get such exception, it's probably a bug which we'll mask,
telegram/chatmember.py
Outdated
def __eq__(self, other): | ||
if isinstance(other, self.__class__): | ||
return self.user == other.user and self.status == other.status | ||
return NotImplemented |
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 will be more appropriate:
return type(self) == type(other) and self.user == other.user and self.status == other.status
(no need to return NotImplemented
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.
mmm... following the comments above.
maybe we should use the same __eq__
from the parent class with a special class like this _get_id()
will return a Tuple of (user, status)?
Great work @bomjacob ! |
Fixes #587 by adding
__eq__
and__hash__
methods to the baseTelegramObject
class to make them work with the==
operator.This makes the following classes behave correctly:
__eq__
implementation, is it still needed?)There's still a few classes that still don't work since they don't have an id. Many of them I don't think needs handling of this (like InlineKeyboard, I don't think anyone ever needs to compare that), but the following we should maybe try to find a solution to. For now I just wanted to get the basics working, to get your opinion on weather this is the way we wanna do it.
We might also wanna have a look at the
ext
classes to see if any of them need__eq__
too.Still lacking tests. Should I test all of them in their respective test_CLASSNAME or should I add a new test_equality.py test?