Add equality rich comparision operators to telegram objects by jsmnbom · Pull Request #604 · python-telegram-bot/python-telegram-bot

jsmnbom · 2017-05-12T17:47:59Z

Fixes #587 by adding __eq__ and __hash__ methods to the base TelegramObject class to make them work with the == operator.
This makes the following classes behave correctly:

Animation
Audio
Bot
CallbackQuery
Chat
ChosenInlineResult
Contact
Document
File
Game
InlineQuery
InlineQueryResult*
Message
PhotoSize (already had a __eq__ implementation, is it still needed?)
Sticker
Update
Video
Voice

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.

ChatMember
Game
GameHighScore
Location
MessageEntity
Venue
WebhookInfo?

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?

Eldinnie · 2017-05-12T18:23:54Z

telegram/base.py

+        raise NotImplementedError
+
+    def __eq__(self, other):
+        if isinstance(other, TelegramObject):


From the Photosize __eq__ it has:
if not isinstance(other, self.__class__):
Might use that here as if isinstance(other, self.__class__):

Yes, good idea :D

jsmnbom · 2017-05-12T18:30:22Z

@tsnoam could you shrine some light on why PhotoSize has a __eq__ method? It was implemented in ec8cd37 for some reason...
I know it was two years ago, so you probably don't :P

Eldinnie · 2017-05-12T18:35:05Z

Seems pretty solid. I added a comment.
I think the photosize could as well be deleted, since it checks size too, but I think if file_id is different the rest is different as well.
for the excluded cases I think maybe implement them in their seperate classes.

ChatMember (user == user and status == status)
Game (title?)
GameHighscore, seems pointless, but for completion?
Location (lat == lat and lon == lon)
MessageEntity Seems pointless
Venue (lat==lat and lon==lon and title==title)
WebhookInfo no...

Regarding tests, I would vote for adding them in their respective classes.

Eldinnie · 2017-05-12T18:43:14Z

@bomjacob @tsnoam For the tests :D see test_video.py line 159 and test_video.py line 159

jsmnbom · 2017-05-12T18:48:58Z

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

tsnoam · 2017-05-12T22:14:28Z

@bomjacob As far as i recall, telegram servers started to return actual data in reply to sending certain messages (sendPhoto from a url?)
So i had to actually compare the received data with what we expected it to be.

I believe that comparing id may be enough but I don't have insight into telegram internals...

j-maas · 2017-05-13T09:35:06Z

I want to point out an alternative to having the equality check for identity:

One could also have two objects, User1<id=1, first_name="Same"> and User2<id=2, first_name="Same">, which are equal in the sense that they have exactly the same "relevant" attributes, but are not identical since they are two different objects (hence having different ids).
I would argue that it still would make sense if User1 == User2 would return true, since they are the same concerning all relevant attributes.
If I wanted to check for identity, I could use User1 is User2, which returns true if the variables reference the exact same instance. Since the id should be unique, there should never be two instances with the same id and thus this check would be reliable.

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 id is perfect. I still wanted to point this out to be able to discuss it.

jsmnbom · 2017-05-13T09:38:18Z

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

j-maas · 2017-05-13T14:10:10Z

But that's exactly what I'd want:

user1 = telegram.User(1, "Same, but not identical")
user2 = telegram.User(2, "Same, but not identical")
user3 = user2  # Identical

I'd expect:

user1 == user2  # True
user1 is user2  # False
user2 == user3  # True
user2 is user3  # True

The critical thing is that there shouldn't be the possibility of creating two objects user1 = telegram.User(1, "Identical") and user2 = telegram.User(1, "Identical") with the same id for this to work perfectly. However, I'm sure that it is in principal possible to create those two objects, since Python would not enforce the id being unique.

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! :)

jsmnbom · 2017-05-13T14:11:41Z

Oh that was what you meant?
Well.. that's pretty much the way to handle these things... you should really never mess with the id implementation :D

Eldinnie · 2017-05-13T14:21:37Z

@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 User instances represent the same user. Checking if they have a similar feature is not what we're trying to achieve.

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.

tsnoam · 2017-05-13T15:10:36Z

The is operator is meant to be used to compare that two objects are exactly the same object, in memory. Changing our implementation to always return (for example) the same User instance for the same user will create undesirable side effects, like, the consumer of User won't be able to change any value inside (not sure why one would want it, but still) without changing it for all other places his program, whether he'd like it or not. (it will be kind of a singleton per user).
At the devel team, we find this behaviour non intuitive and "non pythonic".
Therefor, the implementation would be for a == (equality) operator.

Audio, Document, File, Location, Photo, Sticker, Venue, Video and Voice

j-maas · 2017-05-13T15:32:18Z

@Eldinnie It's not about checking the name. It's more like a = 5 and b = 5 being two different objects (at least for the sake of the argument) but evaluating as a == b # True. The user's id is similar to the memory address of the object: It's a representation for an instance. It would not count into the natural notion of two users being equal (having exactly the same attributes). However, I get what you're getting at: In code, most of the time we want to check if the two users are identical, not if they are the same regarding all relevant (i. e. ignoring id) attributes.

@tsnoam That's a good point against enforcing users to be singletons. That also makes checking by is invalid, leaving only the identity check through ==. 👍

My suggestion was just something that was intuitive to me. I simply feel that == works more like Java's .equals function for strings, in that it should indicate whether objects are the same, not if they are the same instance. This intuition gets really tricky with an id attribute, since from a enduser's perspective, the id is invisible and wouldn't count into their notion of two users being the same.
I totally agree that it is justified to use == for checking if two users are identical from the API's point of view! Let's just keep the implementation above, then. :)

Also thank you for your patience and answers! It's really awesome that you addressed my issue in such detail!

Eldinnie · 2017-05-13T15:52:10Z

@y0hy0h
The problem is that in your example (a=5 b=5) a and b are not the objects, 5 is. a and b are both pointing to that same object.
I still don't understand why you would want two users with different ID evaluate as equal, but maybe it's better to keep this discussion away from this thread.

ChosenInlineResult, InlineQuery, Message, Update and User

tsnoam

See comments on the changes.
Basically, the comments about how to hash/eq/raise are relevant for all types.

tsnoam · 2017-05-13T17:16:03Z

telegram/base.py

+        raise NotImplementedError
+
+    def __eq__(self, other):
+        if isinstance(other, self.__class__):


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.

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.

tsnoam · 2017-05-13T17:16:46Z

telegram/base.py

+        for x in ['id', 'file_id', 'message_id', 'result_id', 'update_id']:
+            if hasattr(self, x):
+                return self.__getattribute__(x)
+        raise NotImplementedError


I prefer RuntimeError to be raised. After all, _get_id is implemented...

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.

tsnoam · 2017-05-13T17:17:34Z

telegram/base.py

+    def _get_id(self):
+        for x in ['id', 'file_id', 'message_id', 'result_id', 'update_id']:
+            if hasattr(self, x):
+                return self.__getattribute__(x)


I usually prefer using getattr(self, x). Any reason you chose to use __getattribute__ here?

Any reason you chose to use __getattribute__ here?

I'd forgotten it existed... I'll change it :D

tsnoam · 2017-05-13T17:19:34Z

telegram/base.py

+    def __eq__(self, other):
+        if isinstance(other, self.__class__):
+            try:
+                return self._get_id() == other._get_id()


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

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.

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.

tsnoam · 2017-05-13T17:21:41Z

telegram/base.py

+        if isinstance(other, self.__class__):
+            try:
+                return self._get_id() == other._get_id()
+            except NotImplementedError:


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.

tsnoam · 2017-05-13T17:22:18Z

telegram/base.py

+                return self._get_id() == other._get_id()
+            except NotImplementedError:
+                pass  # return NotImplemented
+        return NotImplemented


Just return False.

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

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

tsnoam · 2017-05-13T17:23:07Z

telegram/base.py

+        try:
+            return hash((
+                self.__class__,
+                self._get_id(),))


Style nitpick: the entire return statement can be in a single line...

Not according to my yapf

This is what I meant. Cleaner and works with yapf ;-)

def __hash__(self): return hash((self.__class__, self._get_id()))

tsnoam · 2017-05-13T17:24:46Z

telegram/base.py

+            return hash((
+                self.__class__,
+                self._get_id(),))
+        except NotImplementedError:


I don't think catching NotIMplementedError here is needed. If we get such exception, it's probably a bug which we'll mask,

tsnoam · 2017-05-13T17:26:19Z

telegram/chatmember.py

+    def __eq__(self, other):
+        if isinstance(other, self.__class__):
+            return self.user == other.user and self.status == other.status
+        return NotImplemented


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

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

Also add forgotten equality check in test_chat

tsnoam · 2017-05-14T21:28:06Z

Great work @bomjacob !

Add equality rich comparision operator to base class

2b28f12

Eldinnie reviewed May 12, 2017

View reviewed changes

jsmnbom changed the title ~~Add equality rich comparision operator to base class~~ Add equality rich comparision operators to telegram objects May 13, 2017

jsmnbom added 2 commits May 13, 2017 14:47

Use self.__class__ instead of "hardcoding" TelegramObject

63763c6

Contact.phone_number really isn't an id imo

83e3de1

Add equality check and hash to ChatMember, Location and Venue.

c2ac3a8

jsmnbom added 2 commits May 13, 2017 17:26

Hash based on self.__class__

6693054

Equality tests part 1

cd0b996

Audio, Document, File, Location, Photo, Sticker, Venue, Video and Voice

Move AnimationTest and give it some proper values

34ac663

jsmnbom added 7 commits May 13, 2017 18:06

Equality tests for Animation, Contact and uncached inlinequeryresults

65b108c

Equality tests for more types

67e8291

ChosenInlineResult, InlineQuery, Message, Update and User

Equality tests for cached inlinequeryresults

bfe808c

Fix equality tests for photosize and location

faa1e98

Whoops, compare with other not self

ce25412

Whoops, compare with another type

1419561

Add ChatMember test

e596fce

tsnoam requested changes May 13, 2017

View reviewed changes

jsmnbom and others added 5 commits May 13, 2017 19:31

Fix super() call on pu2

90a0a59

Use getattr instead of __getattribute__

590f6f5

Do equality checks via _id_keys

2bb3ab0

Also add forgotten equality check in test_chat

Do equality checks via _id_attrs

98974a1

base.py: It is customary to declare attributes at the head of a class

dc79f40

tsnoam approved these changes May 14, 2017

View reviewed changes

tsnoam merged commit da01601 into master May 14, 2017

tsnoam deleted the fix-equals branch May 14, 2017 21:30

fabaff mentioned this pull request May 22, 2017

Upgrade python-telegram-bot to 6.0.1 home-assistant/core#7704

Merged

pyup-bot mentioned this pull request Jan 22, 2018

Pin python-telegram-bot to latest version 9.0.0 skeledrew/ratot#2

Closed

This was referenced Mar 2, 2018

Pin python-telegram-bot to latest version 10.0.0 skeledrew/ratot#11

Closed

Pin python-telegram-bot to latest version 10.0.1 skeledrew/ratot#12

Merged

github-actions bot locked and limited conversation to collaborators Aug 24, 2020

Conversation

jsmnbom commented May 12, 2017

Uh oh!

Eldinnie May 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsmnbom commented May 12, 2017

Uh oh!

Eldinnie commented May 12, 2017

Uh oh!

Eldinnie commented May 12, 2017

Uh oh!

jsmnbom commented May 12, 2017

Uh oh!

tsnoam commented May 12, 2017

Uh oh!

j-maas commented May 13, 2017

Uh oh!

jsmnbom commented May 13, 2017

Uh oh!

j-maas commented May 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jsmnbom commented May 13, 2017

Uh oh!

Eldinnie commented May 13, 2017

Uh oh!

tsnoam commented May 13, 2017

Uh oh!

j-maas commented May 13, 2017

Uh oh!

Eldinnie commented May 13, 2017

Uh oh!

tsnoam left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsmnbom May 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tsnoam May 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Eldinnie May 12, 2017 •

edited

Loading

j-maas commented May 13, 2017 •

edited

Loading

jsmnbom May 13, 2017 •

edited

Loading

tsnoam May 13, 2017 •

edited

Loading