Make (most) objects comparable #1724

Bibo-Joshi · 2020-01-23T19:59:39Z

Closes #1708

Added _id_attrs to most of the telegram objects (more or less) by the following paradigm:

If object has some sort of ID, just use that
If object has only optional args, use all of them
If object has required arg, use that
If object hast required arg and exactly one of the optionals must be given, use all

Also added the corresponding tests.

On the way I encountered some difficulties:

Objects with nested lists as attributes, which are not hashable: I had to overwrite __hash__ to use a tuple representation of the lists, e.g. InlineKeyboardMarkup
ReplyKeyboardMarkup accepts both strings and KeyboardButtons while converting stings to KB only on to_dict. So I overwrote __equal__ to cope with this
There was no test for WebhookInfo. I added one
I couldn't come up with a clever way to test InputFile.input_media_content for equality (in case an actual file handler is passed, that is), so I left that and InputMedia… out
I have no clue of the passport module. All objects that have an ID already had the _id_attrs set and it seemed to me that all the other classes have no stand alone tests. So I left them out as well …

Needs to be updated, when

API 4.5 API 4.5 #1508
API 4.6 4.6 API update #1723
API 4.7 API 4.7 #1858
API 4.8

are merged

Bibo-Joshi · 2020-01-23T20:25:34Z

equality test for InlineKeyboardMarkup fails on Py2.7. Don't know why, but don't really care 🤷‍♂

Poolitzer · 2020-01-25T04:40:38Z

thanks for asking me to review, but I gonna decline, this is too much for me :D

Bibo-Joshi · 2020-03-09T18:51:11Z

test_set_game_score_1 is the one again …

# Conflicts: # tests/test_inlinekeyboardmarkup.py

Bibo-Joshi · 2020-04-19T11:35:54Z

Just a thought: should we make TelegramObject.__eq__ raise an error or warning, if it's comparing two objects, one of which has an emtpy _id_attrs tuple, i.e. it's not comparable?

Edit: I just went for it (see commit below)

# Conflicts: # telegram/base.py # tests/test_dice.py # tests/test_poll.py

tsnoam

As far as I can tell looks fine.
One question, though.

telegram/base.py

Bibo-Joshi

Went through the _id_attrs it set again and actually found some discussion points. Also I was thinking about if/how this behaviour should be documented. I see different possibilities here:

Don't document. It's a detail for advanced users, who can read code. Also we don't want to make promises
Make a note on TelegramObject along the lines "If the object has an ID, that's the one. If not, it's mostly the required args + meaningful optionals. We can't make promises. Check the code"
Make a note on every object, which attrs are checked.

telegram/forcereply.py

telegram/inline/inputcontactmessagecontent.py

telegram/inline/inputlocationmessagecontent.py

telegram/inline/inputtextmessagecontent.py

telegram/poll.py

Bibo-Joshi · 2020-05-28T18:51:56Z

Added documentation, also for the classes, who already had id attrs.
Addressed some of the above mentioned stuff
In the process, I added message.chat to the id attrs, as message_id is not unique across chats. Strictly speaking this is somewhat breaking. Then again, the behaviour wasn't documented yet, so I'd sleep well with that …
Coday complains: Access to a protected member _id_attrs of a client class: if other._id_attrs == (): That's okay imo.
py3.6 fails on set_game_score …

Poolitzer

Looks good to me.

telegram/inline/inputcontactmessagecontent.py

telegram/inline/inputtextmessagecontent.py

telegram/payment/orderinfo.py

Poolitzer · 2020-06-23T13:33:42Z

Sorry, I forgot the actual issue I have with this: Documentation. I like that you wrote down what gets compared where, but I dont like that being before the attributes of the object. I feel like most users will want to read about attributes rather then what gets compared if they compare this object, which means they have to read over this all the time when they open the docs. What about putting the what-gets-compared paragraph at the end, after the parameters?

Bibo-Joshi · 2020-06-23T13:55:38Z

What about putting the what-gets-compared paragraph at the end, after the parameters?

I fear that it's heard to find it there, especially in the docs of classes with many attributes. And it's only 1-3 lines, so it doesn't take up too much space, imo … The alternative would be to have an explicit docstring for __eq__(), but I don't think that I like that …

telegram/files/venue.py

Bibo-Joshi · 2020-07-13T14:19:39Z

Some concerns about this PR were voiced offline. I'll try to summarize in hopes of not missing the point:

Implementing __eq__() to compare only a subset of a classes attributes will be unintuitive for users
This is especially true for TG objects that can change, e.g. two Message objects will be equal when testing for message_id only, while maybe having different text
Custom comparison by attributes in easy to achive, so there is no reason to implement __eq__()

I'd like to add that this argumentation not only sees harm in this PR but also in the current use of _id_attrs

While I don't agree with this argumentation, I find the arguments important and like to have this properly discussed before going for merge. For completeness sake, here is my line of thought:

We already have __eq__() implemented for objects with some kind of ID. So we could a) remove that capability, b) leave it as is, or c) extend it to all/most objects
The default implementation of __eq__() is useless - I'd rather use is, if I want to compare by object identity. So giving objects a meaningful comparison is crucial imo, which is why I went for c)
Currently this PR aims to set _id_attrs to the attrs that uniquely identify an object, i.e. this comparison is the "correct" one in most use cases
This PR adds documentation, so == is no black box and if users need a different comparison, they can still implement that

# Conflicts: # tests/test_telegramobject.py

Bibo-Joshi · 2020-07-14T19:31:44Z

We discussed the PR in the dev chat again and decided to merge.
I went over the changes once again ad made some minor corrections.
py3.8 fail unrelated, codacy has v13 issues. Merging.

* Make most objects comparable * ID attrs for PollAnswer * fix test_game * fix test_userprofilephotos * update for API 4.7 * Warn on meaningless comparisons * Update for API 4.8 * Address review * Get started on docs, update Message._id_attrs * Change PollOption & InputLocation * Some more changes * Even more changes

Make most objects comparable

5030506

Bibo-Joshi added enhancement ⚙️ tests affected functionality: tests labels Jan 23, 2020

Bibo-Joshi added this to the 12.5 milestone Jan 23, 2020

Bibo-Joshi requested review from Eldinnie and Poolitzer January 23, 2020 19:59

Bibo-Joshi mentioned this pull request Jan 24, 2020

4.6 API update #1723

Merged

2 tasks

Poolitzer removed their request for review January 25, 2020 04:41

Poolitzer added the 📋 pending-review work status: pending-review label Jan 25, 2020

Merge branch 'master' into id_attrs

6087265

Bibo-Joshi added 6 commits March 29, 2020 11:58

Merge branch 'master' into id_attrs

52a4484

ID attrs for PollAnswer

5ac6253

fix test_game

dcb9699

fix test_userprofilephotos

a86136b

Merge branch 'master' into id_attrs

e5be1cf

# Conflicts: # tests/test_inlinekeyboardmarkup.py

update for API 4.7

89c612f

Bibo-Joshi force-pushed the id_attrs branch from ac904c4 to 89c612f Compare April 15, 2020 08:19

Bibo-Joshi mentioned this pull request Apr 20, 2020

Dispatcher.set_commands #1911

Closed

Bibo-Joshi added 4 commits April 25, 2020 20:24

Merge branch 'master' into id_attrs

2e6433a

Warn on meaningless comparisons

2de5abc

Merge branch 'master' into id_attrs

af5fb3e

# Conflicts: # telegram/base.py # tests/test_dice.py # tests/test_poll.py

Update for API 4.8

0f9a5f9

tsnoam reviewed May 27, 2020

View reviewed changes

telegram/base.py Show resolved Hide resolved

Address review

28c1131

Bibo-Joshi commented May 27, 2020

View reviewed changes

Get started on docs, update Message._id_attrs

176ee98

Change PollOption & InputLocation

b18827d

Bibo-Joshi modified the milestones: 12.6, 13.0 Jun 12, 2020

Poolitzer approved these changes Jun 23, 2020

View reviewed changes

telegram/inline/inputcontactmessagecontent.py Outdated Show resolved Hide resolved

telegram/inline/inputtextmessagecontent.py Show resolved Hide resolved

telegram/payment/orderinfo.py Outdated Show resolved Hide resolved

Bibo-Joshi commented Jul 13, 2020

View reviewed changes

telegram/files/venue.py Outdated Show resolved Hide resolved

Bibo-Joshi changed the base branch from master to v13 July 14, 2020 17:24

Bibo-Joshi added 3 commits July 14, 2020 19:30

Merge branch 'v13' into id_attrs

47a8404

# Conflicts: # tests/test_telegramobject.py

Some more changes

2904ae3

Even more changes

81cbf67

Bibo-Joshi merged commit 8855292 into v13 Jul 14, 2020

Bibo-Joshi deleted the id_attrs branch July 14, 2020 19:34

Bibo-Joshi mentioned this pull request Jul 18, 2020

[FEATURE] Add _id_attrs #1708

Closed

github-actions bot locked as resolved and limited conversation to collaborators Aug 17, 2020

Bibo-Joshi added 🔌 enhancement pr description: enhancement and removed enhancement labels Nov 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make (most) objects comparable #1724

Make (most) objects comparable #1724

Bibo-Joshi commented Jan 23, 2020 •

edited

Loading

Bibo-Joshi commented Jan 23, 2020

Poolitzer commented Jan 25, 2020

Bibo-Joshi commented Mar 9, 2020

Bibo-Joshi commented Apr 19, 2020 •

edited

Loading

tsnoam left a comment

Bibo-Joshi left a comment

Bibo-Joshi commented May 28, 2020

Poolitzer left a comment

Poolitzer commented Jun 23, 2020 •

edited

Loading

Bibo-Joshi commented Jun 23, 2020

Bibo-Joshi commented Jul 13, 2020 •

edited

Loading

Bibo-Joshi commented Jul 14, 2020

Make (most) objects comparable #1724

Make (most) objects comparable #1724

Conversation

Bibo-Joshi commented Jan 23, 2020 • edited Loading

Bibo-Joshi commented Jan 23, 2020

Poolitzer commented Jan 25, 2020

Bibo-Joshi commented Mar 9, 2020

Bibo-Joshi commented Apr 19, 2020 • edited Loading

tsnoam left a comment

Choose a reason for hiding this comment

Bibo-Joshi left a comment

Choose a reason for hiding this comment

Bibo-Joshi commented May 28, 2020

Poolitzer left a comment

Choose a reason for hiding this comment

Poolitzer commented Jun 23, 2020 • edited Loading

Bibo-Joshi commented Jun 23, 2020

Bibo-Joshi commented Jul 13, 2020 • edited Loading

Bibo-Joshi commented Jul 14, 2020

Bibo-Joshi commented Jan 23, 2020 •

edited

Loading

Bibo-Joshi commented Apr 19, 2020 •

edited

Loading

Poolitzer commented Jun 23, 2020 •

edited

Loading

Bibo-Joshi commented Jul 13, 2020 •

edited

Loading