Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Simplify Handling of Empty Data inTelegramObject.de_json and Friends#4617

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
Bibo-Joshi merged 17 commits intomasterfromde-json-empty-input
Jan 14, 2025

Conversation

Bibo-Joshi
Copy link
Member

When Ready,closes#4614

Currently, I expect that a bunch of tests will fail as I haven't updated them yet

@Bibo-JoshiBibo-Joshi added 🛠 refactorchange type: refactor 🛠 code-qualitychange type: code-quality labelsDec 29, 2024
@github-actionsgithub-actionsbot removed the 🛠 code-qualitychange type: code-quality labelDec 29, 2024
@codecovCodecov
Copy link

codecovbot commentedDec 29, 2024
edited
Loading

❌ 177 Tests Failed:

Tests completedFailedPassedSkipped
63751776198452
View the top 3 failed tests by shortest run time
tests.test_bot.TestBotWithoutRequest::test_defaults_handling[Bot.get_forum_topic_icon_stickers]
Stack Traces | 0.002s run time
self=<tests.test_bot.TestBotWithoutRequestobjectat0x0000013990675270>bot_class=<class'telegram._bot.Bot'>bot_method_name='get_forum_topic_icon_stickers'bot_method=<boundmethodExtBot.get_forum_topic_icon_stickersofPytestExtBot[token=5737018356:AAH138SuiKQF0LDCWsfgWeXfjJ5d63kCWLA]>offline_bot=PytestExtBot[token=5737018356:AAH138SuiKQF0LDCWsfgWeXfjJ5d63kCWLA]raw_bot=PytestBot[token=5737018356:AAH138SuiKQF0LDCWsfgWeXfjJ5d63kCWLA]@bot_methods(ext_bot=False)asyncdeftest_defaults_handling(self,bot_class,bot_method_name:str,bot_method,offline_bot:PytestExtBot,raw_bot:PytestBot,    ):"""        Here we check that the offline_bot methods handle tg.ext.Defaults correctly. This has two        parts:        1. Check that ExtBot actually inserts the defaults values correctly        2. Check that tg.Bot just replaces `DefaultValue(obj)` with `obj`, i.e. that it doesn't            pass any `DefaultValue` instances to Request. See the docstring of            tg.Bot._insert_defaults for details on why we need that        As for most defaults,        we can't really check the effect, we just check if we're passing the correct kwargs to        Request.post. As offline_bot method tests a scattered across the different test files, we        do this here in one place.        The same test is also run for all the shortcuts (Message.reply_text) etc in the        corresponding tests.        Finally, there are some tests for Defaults.{parse_mode, quote, allow_sending_without_reply}        at the appropriate places, as those are the only things we can actually check.        """# Mocking get_me within check_defaults_handling messes with the cached values like# Bot.{bot, username, id, …}` unless we return the expected User object.return_value= (offline_bot.botifbot_method_name.lower().replace("_","")=="getme"elseNone        )# Check that ExtBot does the right thingbot_method=getattr(offline_bot,bot_method_name)raw_bot_method=getattr(raw_bot,bot_method_name)>assertawaitcheck_defaults_handling(bot_method,offline_bot,return_value=return_value)tests\test_bot.py:500:__ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _tests\auxil\bot_method_checks.py:668:incheck_defaults_handlingraiseexctests\auxil\bot_method_checks.py:636:incheck_defaults_handlingassertawaitmethod(**kwargs)inexpected_return_valuestelegram\ext\_extbot.py:1913:inget_forum_topic_icon_stickersreturnawaitsuper().get_forum_topic_icon_stickers(telegram\_bot.py:8328:inget_forum_topic_icon_stickersreturnSticker.de_list(result,self)_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _cls=<class'telegram._files.sticker.Sticker'>,data=Nonebot=PytestExtBot[token=5737018356:AAH138SuiKQF0LDCWsfgWeXfjJ5d63kCWLA]    @classmethoddefde_list(cls:type[Tele_co],data:list[JSONDict],bot:Optional["Bot"]=None    )->tuple[Tele_co, ...]:"""Converts a list of JSON objects to a tuple of Telegram objects.        .. versionchanged:: 20.0           * Returns a tuple instead of a list.           * Filters out any :obj:`None` values.        Args:            data (list[dict[:obj:`str`, ...]]): The JSON data.            bot (:class:`telegram.Bot`, optional): The bot associated with these object. Defaults                to :obj:`None`, in which case shortcut methods will not be available.                .. versionchanged:: 21.4                   :paramref:`bot` is now optional and defaults to :obj:`None`        Returns:            A tuple of Telegram objects.        """>returntuple(objforobjin (cls.de_json(d,bot)fordindata))ETypeError:'NoneType'objectisnotiterabletelegram\_telegramobject.py:457:TypeError
tests.test_bot.TestBotWithoutRequest::test_defaults_handling[Bot.get_chat]
Stack Traces | 0.003s run time
self=<tests.test_bot.TestBotWithoutRequestobjectat0x0000013990674FF0>bot_class=<class'telegram._bot.Bot'>,bot_method_name='get_chat'bot_method=<boundmethodExtBot.get_chatofPytestExtBot[token=5737018356:AAH138SuiKQF0LDCWsfgWeXfjJ5d63kCWLA]>offline_bot=PytestExtBot[token=5737018356:AAH138SuiKQF0LDCWsfgWeXfjJ5d63kCWLA]raw_bot=PytestBot[token=5737018356:AAH138SuiKQF0LDCWsfgWeXfjJ5d63kCWLA]@bot_methods(ext_bot=False)asyncdeftest_defaults_handling(self,bot_class,bot_method_name:str,bot_method,offline_bot:PytestExtBot,raw_bot:PytestBot,    ):"""        Here we check that the offline_bot methods handle tg.ext.Defaults correctly. This has two        parts:        1. Check that ExtBot actually inserts the defaults values correctly        2. Check that tg.Bot just replaces `DefaultValue(obj)` with `obj`, i.e. that it doesn't            pass any `DefaultValue` instances to Request. See the docstring of            tg.Bot._insert_defaults for details on why we need that        As for most defaults,        we can't really check the effect, we just check if we're passing the correct kwargs to        Request.post. As offline_bot method tests a scattered across the different test files, we        do this here in one place.        The same test is also run for all the shortcuts (Message.reply_text) etc in the        corresponding tests.        Finally, there are some tests for Defaults.{parse_mode, quote, allow_sending_without_reply}        at the appropriate places, as those are the only things we can actually check.        """# Mocking get_me within check_defaults_handling messes with the cached values like# Bot.{bot, username, id, …}` unless we return the expected User object.return_value= (offline_bot.botifbot_method_name.lower().replace("_","")=="getme"elseNone        )# Check that ExtBot does the right thingbot_method=getattr(offline_bot,bot_method_name)raw_bot_method=getattr(raw_bot,bot_method_name)>assertawaitcheck_defaults_handling(bot_method,offline_bot,return_value=return_value)tests\test_bot.py:500:__ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _tests\auxil\bot_method_checks.py:668:incheck_defaults_handlingraiseexctests\auxil\bot_method_checks.py:636:incheck_defaults_handlingassertawaitmethod(**kwargs)inexpected_return_valuestelegram\ext\_extbot.py:889:inget_chatresult=awaitsuper().get_chat(telegram\_bot.py:4732:inget_chatreturnChatFullInfo.de_json(result,self)telegram\_chatfullinfo.py:517:inde_jsondata=cls._parse_data(data)_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _data=None    @staticmethoddef_parse_data(data:JSONDict)->JSONDict:"""Should be called by subclasses that override de_json to ensure that the input        is not altered. Whoever calls de_json might still want to use the original input        for something else.        """>returndata.copy()EAttributeError:'NoneType'objecthasnoattribute'copy'telegram\_telegramobject.py:385:AttributeError
tests.test_bot.TestBotWithoutRequest::test_defaults_handling[Bot.get_star_transactions]
Stack Traces | 0.003s run time
self=<tests.test_bot.TestBotWithoutRequestobjectat0x0000013990675590>bot_class=<class'telegram._bot.Bot'>bot_method_name='get_star_transactions'bot_method=<boundmethodExtBot.get_star_transactionsofPytestExtBot[token=5737018356:AAH138SuiKQF0LDCWsfgWeXfjJ5d63kCWLA]>offline_bot=PytestExtBot[token=5737018356:AAH138SuiKQF0LDCWsfgWeXfjJ5d63kCWLA]raw_bot=PytestBot[token=5737018356:AAH138SuiKQF0LDCWsfgWeXfjJ5d63kCWLA]@bot_methods(ext_bot=False)asyncdeftest_defaults_handling(self,bot_class,bot_method_name:str,bot_method,offline_bot:PytestExtBot,raw_bot:PytestBot,    ):"""        Here we check that the offline_bot methods handle tg.ext.Defaults correctly. This has two        parts:        1. Check that ExtBot actually inserts the defaults values correctly        2. Check that tg.Bot just replaces `DefaultValue(obj)` with `obj`, i.e. that it doesn't            pass any `DefaultValue` instances to Request. See the docstring of            tg.Bot._insert_defaults for details on why we need that        As for most defaults,        we can't really check the effect, we just check if we're passing the correct kwargs to        Request.post. As offline_bot method tests a scattered across the different test files, we        do this here in one place.        The same test is also run for all the shortcuts (Message.reply_text) etc in the        corresponding tests.        Finally, there are some tests for Defaults.{parse_mode, quote, allow_sending_without_reply}        at the appropriate places, as those are the only things we can actually check.        """# Mocking get_me within check_defaults_handling messes with the cached values like# Bot.{bot, username, id, …}` unless we return the expected User object.return_value= (offline_bot.botifbot_method_name.lower().replace("_","")=="getme"elseNone        )# Check that ExtBot does the right thingbot_method=getattr(offline_bot,bot_method_name)raw_bot_method=getattr(raw_bot,bot_method_name)>assertawaitcheck_defaults_handling(bot_method,offline_bot,return_value=return_value)tests\test_bot.py:500:__ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _tests\auxil\bot_method_checks.py:668:incheck_defaults_handlingraiseexctests\auxil\bot_method_checks.py:636:incheck_defaults_handlingassertawaitmethod(**kwargs)inexpected_return_valuestelegram\ext\_extbot.py:4309:inget_star_transactionsreturnawaitsuper().get_star_transactions(telegram\_bot.py:9405:inget_star_transactionsreturnStarTransactions.de_json(telegram\_payment\stars\startransactions.py:159:inde_jsondata=cls._parse_data(data)_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _data=None    @staticmethoddef_parse_data(data:JSONDict)->JSONDict:"""Should be called by subclasses that override de_json to ensure that the input        is not altered. Whoever calls de_json might still want to use the original input        for something else.        """>returndata.copy()EAttributeError:'NoneType'objecthasnoattribute'copy'telegram\_telegramobject.py:385:AttributeError

To view more test analytics, go to theTest Analytics Dashboard
📢 Thoughts on this report?Let us know!

@Bibo-Joshi
Copy link
MemberAuthor

Admittedly, this turned out to be a bigger change than expected. Not really the removal ofif not data: return None itself, but all that depended on that. However, this shows how much the internal logic ofde_json and our tests dependet on this undocumented implementation detail that has no user facing benefit. Clearing that up and making the business logic independent of satisfying minor convenience needs for the tests is a good change IMO.

@Bibo-JoshiBibo-Joshi marked this pull request as ready for reviewDecember 30, 2024 14:42
Copy link
Member

@harshil21harshil21 left a comment

Choose a reason for hiding this comment

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

just a couple of questions-

@Bibo-JoshiBibo-Joshi added the 📋 do-not-merge-yetwork status: do-not-merge-yet labelJan 2, 2025
@Bibo-JoshiBibo-Joshi removed the 📋 do-not-merge-yetwork status: do-not-merge-yet labelJan 5, 2025
Copy link
Member

@harshil21harshil21 left a comment

Choose a reason for hiding this comment

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

LGTM. Just one minor thing to update:

@Bibo-JoshiBibo-Joshi merged commitdd592cd intomasterJan 14, 2025
26 checks passed
@Bibo-JoshiBibo-Joshi deleted the de-json-empty-input branchJanuary 14, 2025 16:12
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsJan 22, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@harshil21harshil21harshil21 approved these changes

@PoolitzerPoolitzerAwaiting requested review from Poolitzer

Assignees
No one assigned
Labels
🛠 refactorchange type: refactor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Simplify Handling of empty data inTO.de_json
2 participants
@Bibo-Joshi@harshil21

[8]ページ先頭

©2009-2025 Movatter.jp