Skip to content
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

Merged
merged 19 commits into from
May 14, 2017
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
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
22 changes: 22 additions & 0 deletions telegram/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -80,3 +80,25 @@ def to_dict(self):
data[key] = value

return data

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

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?

Copy link
Member Author

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

raise NotImplementedError
Copy link
Member

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

Copy link
Member Author

@jsmnbom jsmnbom May 13, 2017

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.


def __eq__(self, other):
if isinstance(other, self.__class__):
Copy link
Member

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.

Copy link
Member Author

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.

try:
return self._get_id() == other._get_id()
Copy link
Member

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

Copy link
Member Author

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.

Copy link
Member Author

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.

except NotImplementedError:
Copy link
Member

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.

pass # return NotImplemented
return NotImplemented
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just return False.

Copy link
Member Author

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

Copy link
Member

@tsnoam tsnoam May 13, 2017

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


def __hash__(self):
try:
return hash((
self.__class__,
self._get_id(),))
Copy link
Member

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

Copy link
Member Author

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

Copy link
Member

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

except NotImplementedError:
Copy link
Member

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,

return super().__hash__()
8 changes: 8 additions & 0 deletions telegram/chatmember.py
Original file line number Diff line number Diff line change
Expand Up @@ -64,3 +64,11 @@ def de_json(data, bot):
data['user'] = User.de_json(data.get('user'), bot)

return ChatMember(**data)

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

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

Copy link
Member

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


def __hash__(self):
return hash((self.__class__, self.user, self.status))
8 changes: 8 additions & 0 deletions telegram/contact.py
Original file line number Diff line number Diff line change
Expand Up @@ -61,3 +61,11 @@ def de_json(data, bot):
return None

return Contact(**data)

def __eq__(self, other):
if isinstance(other, self.__class__):
return self.phone_number == other.phone_number
return NotImplemented

def __hash__(self):
return hash(self.phone_number)
8 changes: 8 additions & 0 deletions telegram/location.py
Original file line number Diff line number Diff line change
Expand Up @@ -52,3 +52,11 @@ def de_json(data, bot):
return None

return Location(**data)

def __eq__(self, other):
if isinstance(other, self.__class__):
return self.longitude == other.longitude and self.latitude == other.latitude
return NotImplemented

def __hash__(self):
return hash((self.__class__, self.longitude, self.latitude))
6 changes: 0 additions & 6 deletions telegram/photosize.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,12 +48,6 @@ def __init__(self, file_id, width, height, file_size=None, **kwargs):
# Optionals
self.file_size = file_size

def __eq__(self, other):
if not isinstance(other, self.__class__):
return False
return (self.file_id == other.file_id and self.width == other.width
and self.height == other.height and self.file_size == other.file_size)

@staticmethod
def de_json(data, bot):
"""
Expand Down
8 changes: 8 additions & 0 deletions telegram/venue.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,3 +50,11 @@ def de_json(data, bot):
data['location'] = Location.de_json(data.get('location'), bot)

return Venue(**data)

def __eq__(self, other):
if isinstance(other, self.__class__):
return self.location == other.location and self.title == other.title
return NotImplemented

def __hash__(self):
return hash((self.__class__, self.location, self.title))
91 changes: 91 additions & 0 deletions tests/test_animation.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
#!/usr/bin/env python
#
# A library that provides a Python interface to the Telegram Bot API
# Copyright (C) 2015-2016
# Leandro Toledo de Souza <devs@python-telegram-bot.org>
#
# This program is free software: you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
# the Free Software Foundation, either version 3 of the License, or
# (at your option) any later version.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with this program. If not, see [http://www.gnu.org/licenses/].
"""This module contains an object that represents Tests for Telegram Animation"""

import unittest
import sys

sys.path.append('.')

import telegram
from tests.base import BaseTest


class AnimationTest(BaseTest, unittest.TestCase):
"""This object represents Tests for Telegram Animation."""

def setUp(self):
self.animation_file_id = 'CgADBAADFQEAAny4rAUgukhiTv2TWwI'
self.thumb = telegram.PhotoSize.de_json({
'height': 50,
'file_size': 1613,
'file_id': 'AAQEABPQUWQZAAT7gZuQAAH0bd93VwACAg',
'width': 90
}, self._bot)
self.file_name = "game.gif.mp4"
self.mime_type = "video/mp4"
self.file_size = 4008

self.json_dict = {
'file_id': self.animation_file_id,
'thumb': self.thumb.to_dict(),
'file_name': self.file_name,
'mime_type': self.mime_type,
'file_size': self.file_size
}

def test_animation_de_json(self):
animation = telegram.Animation.de_json(self.json_dict, self._bot)

self.assertEqual(animation.file_id, self.animation_file_id)
self.assertEqual(animation.thumb, self.thumb)
self.assertEqual(animation.file_name, self.file_name)
self.assertEqual(animation.mime_type, self.mime_type)
self.assertEqual(animation.file_size, self.file_size)

def test_animation_to_json(self):
animation = telegram.Animation.de_json(self.json_dict, self._bot)

self.assertTrue(self.is_json(animation.to_json()))

def test_animation_to_dict(self):
animation = telegram.Animation.de_json(self.json_dict, self._bot)

self.assertTrue(self.is_dict(animation.to_dict()))
self.assertEqual(animation['file_id'], self.animation_file_id)
self.assertEqual(animation['thumb'], self.thumb)
self.assertEqual(animation['file_name'], self.file_name)
self.assertEqual(animation['mime_type'], self.mime_type)
self.assertEqual(animation['file_size'], self.file_size)

def test_equality(self):
a = telegram.Animation(self.animation_file_id)
b = telegram.Animation(self.animation_file_id)
d = telegram.Animation("")
e = telegram.Voice(self.animation_file_id, 0)

self.assertEqual(a, b)
self.assertEqual(hash(a), hash(b))
self.assertIsNot(a, b)

self.assertNotEqual(a, d)
self.assertNotEqual(hash(a), hash(d))

self.assertNotEqual(a, e)
self.assertNotEqual(hash(a), hash(e))
20 changes: 20 additions & 0 deletions tests/test_audio.py
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,26 @@ def test_reply_audio(self):

self.assertNotEqual(message.audio.file_id, None)

def test_equality(self):
a = telegram.Audio(self.audio_file_id, self.duration)
b = telegram.Audio(self.audio_file_id, self.duration)
c = telegram.Audio(self.audio_file_id, 0)
d = telegram.Audio("", self.duration)
e = telegram.Voice(self.audio_file_id, self.duration)

self.assertEqual(a, b)
self.assertEqual(hash(a), hash(b))
self.assertIsNot(a, b)

self.assertEqual(a, c)
self.assertEqual(hash(a), hash(c))

self.assertNotEqual(a, d)
self.assertNotEqual(hash(a), hash(d))

self.assertNotEqual(a, e)
self.assertNotEqual(hash(a), hash(e))


if __name__ == '__main__':
unittest.main()
20 changes: 20 additions & 0 deletions tests/test_choseninlineresult.py
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,26 @@ def test_choseninlineresult_to_dict(self):
self.assertEqual(result['from'], self.from_user.to_dict())
self.assertEqual(result['query'], self.query)

def test_equality(self):
a = telegram.ChosenInlineResult(self.result_id, None, "Query", "")
b = telegram.ChosenInlineResult(self.result_id, None, "Query", "")
c = telegram.ChosenInlineResult(self.result_id, None, "", "")
d = telegram.ChosenInlineResult("", None, "Query", "")
e = telegram.Voice(self.result_id, 0)

self.assertEqual(a, b)
self.assertEqual(hash(a), hash(b))
self.assertIsNot(a, b)

self.assertEqual(a, c)
self.assertEqual(hash(a), hash(c))

self.assertNotEqual(a, d)
self.assertNotEqual(hash(a), hash(d))

self.assertNotEqual(a, e)
self.assertNotEqual(hash(a), hash(e))


if __name__ == '__main__':
unittest.main()
20 changes: 20 additions & 0 deletions tests/test_contact.py
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,26 @@ def test_contact_to_dict(self):
self.assertEqual(contact['last_name'], self.last_name)
self.assertEqual(contact['user_id'], self.user_id)

def test_equality(self):
a = telegram.Contact(self.phone_number, self.first_name)
b = telegram.Contact(self.phone_number, self.first_name)
c = telegram.Contact(self.phone_number, "")
d = telegram.Contact("", self.first_name)
e = telegram.Voice("", 0)

self.assertEqual(a, b)
self.assertEqual(hash(a), hash(b))
self.assertIsNot(a, b)

self.assertEqual(a, c)
self.assertEqual(hash(a), hash(c))

self.assertNotEqual(a, d)
self.assertNotEqual(hash(a), hash(d))

self.assertNotEqual(a, e)
self.assertNotEqual(hash(a), hash(e))


''' Commented out, because it would cause "Too Many Requests (429)" errors.
@flaky(3, 1)
Expand Down
16 changes: 16 additions & 0 deletions tests/test_document.py
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,22 @@ def test_reply_document(self):

self.assertNotEqual(message.document.file_id, '')

def test_equality(self):
a = telegram.Document(self.document_file_id)
b = telegram.Document(self.document_file_id)
d = telegram.Document("")
e = telegram.Voice(self.document_file_id, 0)

self.assertEqual(a, b)
self.assertEqual(hash(a), hash(b))
self.assertIsNot(a, b)

self.assertNotEqual(a, d)
self.assertNotEqual(hash(a), hash(d))

self.assertNotEqual(a, e)
self.assertNotEqual(hash(a), hash(e))


if __name__ == '__main__':
unittest.main()
20 changes: 20 additions & 0 deletions tests/test_file.py
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,26 @@ def test_error_file_without_required_args(self):

self.assertRaises(TypeError, lambda: self._bot.getFile(**json_dict))

def test_equality(self):
a = telegram.File(self.audio_file_id, self._bot)
b = telegram.File(self.audio_file_id, self._bot)
c = telegram.File(self.audio_file_id, None)
d = telegram.File(self.document_file_id, self._bot)
e = telegram.Voice(self.audio_file_id, 0)

self.assertEqual(a, b)
self.assertEqual(hash(a), hash(b))
self.assertIsNot(a, b)

self.assertEqual(a, c)
self.assertEqual(hash(a), hash(c))

self.assertNotEqual(a, d)
self.assertNotEqual(hash(a), hash(d))

self.assertNotEqual(a, e)
self.assertNotEqual(hash(a), hash(e))


if __name__ == '__main__':
unittest.main()
33 changes: 0 additions & 33 deletions tests/test_game.py
Original file line number Diff line number Diff line change
Expand Up @@ -99,36 +99,3 @@ def test_parse_entities(self):
self.assertDictEqual(game.parse_text_entities(),
{entity: 'http://google.com',
entity_2: 'h'})


class AnimationTest(BaseTest, unittest.TestCase):
"""This object represents Tests for Telegram Animatiion."""

def setUp(self):
self.file_id = 'thisisafileid'
self.thumb = {'width': 640, 'height': 360, 'file_id': 'Blah', 'file_size': 0}
self.file_name = 'File name'
self.mime_type = 'something/gif'
self.file_size = 42

self.json_dict = {
'file_id': self.file_id,
'thumb': self.thumb,
'file_name': self.file_name,
'mime_type': self.mime_type,
'file_size': self.file_size
}

def test_animation_de_json(self):
animation = telegram.Animation.de_json(self.json_dict, self._bot)

self.assertEqual(animation.file_id, self.file_id)
self.assertTrue(isinstance(animation.thumb, telegram.PhotoSize))
self.assertEqual(animation.file_name, self.file_name)
self.assertEqual(animation.mime_type, self.mime_type)
self.assertEqual(animation.file_size, self.file_size)

def test_game_to_json(self):
animation = telegram.Animation.de_json(self.json_dict, self._bot)

self.assertTrue(self.is_json(animation.to_json()))
22 changes: 21 additions & 1 deletion tests/test_inlinequery.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ def setUp(self):
user = telegram.User(1, 'First name')
location = telegram.Location(8.8, 53.1)

self._id = 'id'
self._id = 1234
self.from_user = user
self.query = 'query text'
self.offset = 'offset'
Expand Down Expand Up @@ -69,6 +69,26 @@ def test_inlinequery_to_dict(self):
self.assertTrue(self.is_dict(inlinequery))
self.assertDictEqual(inlinequery, self.json_dict)

def test_equality(self):
a = telegram.InlineQuery(self._id, telegram.User(1, ""), "", "")
b = telegram.InlineQuery(self._id, telegram.User(1, ""), "", "")
c = telegram.InlineQuery(self._id, telegram.User(0, ""), "", "")
d = telegram.InlineQuery(0, telegram.User(1, ""), "", "")
e = telegram.Update(self._id)

self.assertEqual(a, b)
self.assertEqual(hash(a), hash(b))
self.assertIsNot(a, b)

self.assertEqual(a, c)
self.assertEqual(hash(a), hash(c))

self.assertNotEqual(a, d)
self.assertNotEqual(hash(a), hash(d))

self.assertNotEqual(a, e)
self.assertNotEqual(hash(a), hash(e))


if __name__ == '__main__':
unittest.main()
Loading