From 9d93417d9a4ec2b5fe79a9d0d0678cf24a1136d6 Mon Sep 17 00:00:00 2001 From: Aksh Gupta Date: Mon, 5 Apr 2021 16:55:27 +0530 Subject: [PATCH] Improve Code Quality (#2450) * chore: refactor code quality issues * Add comment for removing assert statements * Remove deepsource config file * Fix Coverage Co-authored-by: Hinrich Mahler --- docs/source/conf.py | 1 - examples/echobot.py | 2 +- setup.py | 2 -- telegram/base.py | 2 +- telegram/bot.py | 5 ++++- telegram/ext/commandhandler.py | 6 +++--- telegram/ext/conversationhandler.py | 8 ++++---- telegram/ext/dispatcher.py | 2 +- telegram/ext/handler.py | 2 +- telegram/ext/picklepersistence.py | 4 ++-- telegram/games/game.py | 2 +- telegram/message.py | 10 +++++----- telegram/utils/request.py | 2 +- tests/test_bot.py | 3 +++ tests/test_chat.py | 6 +++--- tests/test_commandhandler.py | 2 +- tests/test_conversationhandler.py | 2 +- tests/test_dispatcher.py | 2 +- tests/test_error.py | 2 +- tests/test_persistence.py | 12 ++++++++++++ 20 files changed, 46 insertions(+), 31 deletions(-) diff --git a/docs/source/conf.py b/docs/source/conf.py index d08f120e9..cb75bfece 100644 --- a/docs/source/conf.py +++ b/docs/source/conf.py @@ -13,7 +13,6 @@ # serve to show the default. import sys import os -import shlex # import telegram # If extensions (or modules to document with autodoc) are in another directory, diff --git a/examples/echobot.py b/examples/echobot.py index 29fca22cd..e7c4f1eba 100644 --- a/examples/echobot.py +++ b/examples/echobot.py @@ -34,7 +34,7 @@ def start(update: Update, _: CallbackContext) -> None: """Send a message when the command /start is issued.""" user = update.effective_user update.message.reply_markdown_v2( - f'Hi {user.mention_markdown_v2()}\!', + fr'Hi {user.mention_markdown_v2()}\!', reply_markup=ForceReply(selective=True), ) diff --git a/setup.py b/setup.py index d847fff8a..acffecc18 100644 --- a/setup.py +++ b/setup.py @@ -1,7 +1,5 @@ #!/usr/bin/env python """The setup and build script for the python-telegram-bot library.""" - -import codecs import os import subprocess import sys diff --git a/telegram/base.py b/telegram/base.py index 030ea7b78..4009b49ca 100644 --- a/telegram/base.py +++ b/telegram/base.py @@ -76,7 +76,7 @@ class TelegramObject: return json.dumps(self.to_dict()) def to_dict(self) -> JSONDict: - data = dict() + data = {} for key in iter(self.__dict__): if key == 'bot' or key.startswith('_'): diff --git a/telegram/bot.py b/telegram/bot.py index 37488e7c9..54fd87abe 100644 --- a/telegram/bot.py +++ b/telegram/bot.py @@ -3457,7 +3457,10 @@ class Bot(TelegramObject): data: JSONDict = {'shipping_query_id': shipping_query_id, 'ok': ok} if ok: - assert shipping_options + if not shipping_options: + # not using an assert statement directly here since they are removed in + # the optimized bytecode + raise AssertionError data['shipping_options'] = [option.to_dict() for option in shipping_options] if error_message is not None: data['error_message'] = error_message diff --git a/telegram/ext/commandhandler.py b/telegram/ext/commandhandler.py index 4b38b4994..10a40ba4f 100644 --- a/telegram/ext/commandhandler.py +++ b/telegram/ext/commandhandler.py @@ -354,9 +354,9 @@ class PrefixHandler(CommandHandler): run_async: Union[bool, DefaultValue] = DEFAULT_FALSE, ): - self._prefix: List[str] = list() - self._command: List[str] = list() - self._commands: List[str] = list() + self._prefix: List[str] = [] + self._command: List[str] = [] + self._commands: List[str] = [] super().__init__( 'nocommand', diff --git a/telegram/ext/conversationhandler.py b/telegram/ext/conversationhandler.py index ae615935c..beb299597 100644 --- a/telegram/ext/conversationhandler.py +++ b/telegram/ext/conversationhandler.py @@ -241,9 +241,9 @@ class ConversationHandler(Handler[Update]): Set by dispatcher""" self._map_to_parent = map_to_parent - self.timeout_jobs: Dict[Tuple[int, ...], 'Job'] = dict() + self.timeout_jobs: Dict[Tuple[int, ...], 'Job'] = {} self._timeout_jobs_lock = Lock() - self._conversations: ConversationDict = dict() + self._conversations: ConversationDict = {} self._conversations_lock = Lock() self.logger = logging.getLogger(__name__) @@ -257,7 +257,7 @@ class ConversationHandler(Handler[Update]): "since message IDs are not globally unique." ) - all_handlers: List[Handler] = list() + all_handlers: List[Handler] = [] all_handlers.extend(entry_points) all_handlers.extend(fallbacks) @@ -407,7 +407,7 @@ class ConversationHandler(Handler[Update]): chat = update.effective_chat user = update.effective_user - key = list() + key = [] if self.per_chat: key.append(chat.id) # type: ignore[union-attr] diff --git a/telegram/ext/dispatcher.py b/telegram/ext/dispatcher.py index ed20a6148..0baacbaed 100644 --- a/telegram/ext/dispatcher.py +++ b/telegram/ext/dispatcher.py @@ -511,7 +511,7 @@ class Dispatcher: handler.conversations = self.persistence.get_conversations(handler.name) if group not in self.handlers: - self.handlers[group] = list() + self.handlers[group] = [] self.groups.append(group) self.groups = sorted(self.groups) diff --git a/telegram/ext/handler.py b/telegram/ext/handler.py index 05fd4645d..d4bc46f64 100644 --- a/telegram/ext/handler.py +++ b/telegram/ext/handler.py @@ -202,7 +202,7 @@ class Handler(Generic[UT], ABC): check_result: The result from check_update """ - optional_args: Dict[str, object] = dict() + optional_args: Dict[str, object] = {} if self.pass_update_queue: optional_args['update_queue'] = dispatcher.update_queue diff --git a/telegram/ext/picklepersistence.py b/telegram/ext/picklepersistence.py index f5d5101b3..72a40a759 100644 --- a/telegram/ext/picklepersistence.py +++ b/telegram/ext/picklepersistence.py @@ -106,7 +106,7 @@ class PicklePersistence(BasePersistence): self.bot_data = data.get('bot_data', {}) self.conversations = data['conversations'] except OSError: - self.conversations = dict() + self.conversations = {} self.user_data = defaultdict(dict) self.chat_data = defaultdict(dict) self.bot_data = {} @@ -233,7 +233,7 @@ class PicklePersistence(BasePersistence): new_state (:obj:`tuple` | :obj:`any`): The new state for the given key. """ if not self.conversations: - self.conversations = dict() + self.conversations = {} if self.conversations.setdefault(name, {}).get(key) == new_state: return self.conversations[name][key] = new_state diff --git a/telegram/games/game.py b/telegram/games/game.py index 2d67a490a..c3952f9f1 100644 --- a/telegram/games/game.py +++ b/telegram/games/game.py @@ -83,7 +83,7 @@ class Game(TelegramObject): self.photo = photo # Optionals self.text = text - self.text_entities = text_entities or list() + self.text_entities = text_entities or [] self.animation = animation self._id_attrs = (self.title, self.description, self.photo) diff --git a/telegram/message.py b/telegram/message.py index 3ec746af5..30d2dbc44 100644 --- a/telegram/message.py +++ b/telegram/message.py @@ -438,12 +438,12 @@ class Message(TelegramObject): self.reply_to_message = reply_to_message self.edit_date = edit_date self.text = text - self.entities = entities or list() - self.caption_entities = caption_entities or list() + self.entities = entities or [] + self.caption_entities = caption_entities or [] self.audio = audio self.game = game self.document = document - self.photo = photo or list() + self.photo = photo or [] self.sticker = sticker self.video = video self.voice = voice @@ -452,10 +452,10 @@ class Message(TelegramObject): self.contact = contact self.location = location self.venue = venue - self.new_chat_members = new_chat_members or list() + self.new_chat_members = new_chat_members or [] self.left_chat_member = left_chat_member self.new_chat_title = new_chat_title - self.new_chat_photo = new_chat_photo or list() + self.new_chat_photo = new_chat_photo or [] self.delete_chat_photo = bool(delete_chat_photo) self.group_chat_created = bool(group_chat_created) self.supergroup_chat_created = bool(supergroup_chat_created) diff --git a/telegram/utils/request.py b/telegram/utils/request.py index 9aea76ef3..f34c4f653 100644 --- a/telegram/utils/request.py +++ b/telegram/utils/request.py @@ -119,7 +119,7 @@ class Request: read_timeout: float = 5.0, ): if urllib3_proxy_kwargs is None: - urllib3_proxy_kwargs = dict() + urllib3_proxy_kwargs = {} self._connect_timeout = connect_timeout diff --git a/tests/test_bot.py b/tests/test_bot.py index 2bf26b13d..37bd65226 100644 --- a/tests/test_bot.py +++ b/tests/test_bot.py @@ -1442,6 +1442,9 @@ class TestBot: with pytest.raises(TelegramError, match='should not be empty and there should not be'): bot.answer_shipping_query(1, True) + with pytest.raises(AssertionError): + bot.answer_shipping_query(1, True, shipping_options=[]) + # TODO: Needs improvement. Need incoming pre checkout queries to test def test_answer_pre_checkout_query_ok(self, monkeypatch, bot): # For now just test that our internals pass the correct data diff --git a/tests/test_chat.py b/tests/test_chat.py index bde3883f7..bd964a7eb 100644 --- a/tests/test_chat.py +++ b/tests/test_chat.py @@ -213,7 +213,7 @@ class TestChat: def make_assertion(*_, **kwargs): chat_id = kwargs['chat_id'] == chat.id user_id = kwargs['user_id'] == 42 - o_i_b = kwargs.get('only_if_banned', None) == only_if_banned + o_i_b = kwargs.get('only_if_banned') == only_if_banned return chat_id and user_id and o_i_b assert check_shortcut_signature(Chat.unban_member, Bot.unban_chat_member, ['chat_id'], []) @@ -228,7 +228,7 @@ class TestChat: def make_assertion(*_, **kwargs): chat_id = kwargs['chat_id'] == chat.id user_id = kwargs['user_id'] == 42 - o_i_b = kwargs.get('is_anonymous', None) == is_anonymous + o_i_b = kwargs.get('is_anonymous') == is_anonymous return chat_id and user_id and o_i_b assert check_shortcut_signature( @@ -246,7 +246,7 @@ class TestChat: def make_assertion(*_, **kwargs): chat_id = kwargs['chat_id'] == chat.id user_id = kwargs['user_id'] == 42 - o_i_b = kwargs.get('permissions', None) == permissions + o_i_b = kwargs.get('permissions') == permissions return chat_id and user_id and o_i_b assert check_shortcut_signature( diff --git a/tests/test_commandhandler.py b/tests/test_commandhandler.py index a2cd81ec2..e6ac81911 100644 --- a/tests/test_commandhandler.py +++ b/tests/test_commandhandler.py @@ -79,7 +79,7 @@ class BaseTest: def make_callback_for(self, pass_keyword): def callback(bot, update, **kwargs): - self.test_flag = kwargs.get(keyword, None) is not None + self.test_flag = kwargs.get(keyword) is not None keyword = pass_keyword[5:] return callback diff --git a/tests/test_conversationhandler.py b/tests/test_conversationhandler.py index f8db5dafa..28a6dd130 100644 --- a/tests/test_conversationhandler.py +++ b/tests/test_conversationhandler.py @@ -99,7 +99,7 @@ class TestConversationHandler: def reset(self): self.raise_dp_handler_stop = False self.test_flag = False - self.current_state = dict() + self.current_state = {} self.entry_points = [CommandHandler('start', self.start)] self.states = { self.THIRSTY: [CommandHandler('brew', self.brew), CommandHandler('wait', self.start)], diff --git a/tests/test_dispatcher.py b/tests/test_dispatcher.py index 41a6bbaff..83550e689 100644 --- a/tests/test_dispatcher.py +++ b/tests/test_dispatcher.py @@ -569,7 +569,7 @@ class TestDispatcher: self.store_bot_data = True def get_bot_data(self): - return dict() + return {} def update_bot_data(self, data): raise Exception diff --git a/tests/test_error.py b/tests/test_error.py index 02273e519..1dc664a1c 100644 --- a/tests/test_error.py +++ b/tests/test_error.py @@ -131,7 +131,7 @@ class TestErrors: """ def make_assertion(cls): - assert {sc for sc in cls.__subclasses__()} == covered_subclasses[cls] + assert set(cls.__subclasses__()) == covered_subclasses[cls] for subcls in cls.__subclasses__(): make_assertion(subcls) diff --git a/tests/test_persistence.py b/tests/test_persistence.py index 2d59216a2..6b00046df 100644 --- a/tests/test_persistence.py +++ b/tests/test_persistence.py @@ -960,10 +960,16 @@ class TestPickelPersistence: assert not pickle_persistence.conversations['name1'] == conversation1 pickle_persistence.update_conversation('name1', (123, 123), 5) assert pickle_persistence.conversations['name1'] == conversation1 + assert pickle_persistence.get_conversations('name1') == conversation1 with open('pickletest_conversations', 'rb') as f: conversations_test = defaultdict(dict, pickle.load(f)) assert conversations_test['name1'] == conversation1 + pickle_persistence.conversations = None + pickle_persistence.update_conversation('name1', (123, 123), 5) + assert pickle_persistence.conversations['name1'] == {(123, 123): 5} + assert pickle_persistence.get_conversations('name1') == {(123, 123): 5} + def test_updating_single_file(self, pickle_persistence, good_pickle_files): pickle_persistence.single_file = True @@ -999,10 +1005,16 @@ class TestPickelPersistence: assert not pickle_persistence.conversations['name1'] == conversation1 pickle_persistence.update_conversation('name1', (123, 123), 5) assert pickle_persistence.conversations['name1'] == conversation1 + assert pickle_persistence.get_conversations('name1') == conversation1 with open('pickletest', 'rb') as f: conversations_test = defaultdict(dict, pickle.load(f)['conversations']) assert conversations_test['name1'] == conversation1 + pickle_persistence.conversations = None + pickle_persistence.update_conversation('name1', (123, 123), 5) + assert pickle_persistence.conversations['name1'] == {(123, 123): 5} + assert pickle_persistence.get_conversations('name1') == {(123, 123): 5} + def test_save_on_flush_multi_files(self, pickle_persistence, good_pickle_files): # Should run without error pickle_persistence.flush()