GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Simplify Handling of Empty Data in `TelegramObject.de_json` and Friends by Bibo-Joshi · Pull Request #4617 · python-telegram-bot/python-telegram-bot

Conversation

@Bibo-Joshi

@Bibo-Joshi Bibo-Joshi commented Dec 29, 2024

Copy link
Copy Markdown
Member

When Ready, closes #4614

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@Bibo-Joshi
Get Started on Simplifying de_json/list
e25e5f0
@Bibo-Joshi Bibo-Joshi added 🛠 refactor change type: refactor 🛠 code-quality change type: code-quality labels Dec 29, 2024
@github-actions github-actions Bot removed the 🛠 code-quality change type: code-quality label Dec 29, 2024
@Bibo-Joshi
Merge branch 'master' into de-json-empty-input
2c6913a
@codecov

codecov Bot commented Dec 29, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

❌ 177 Tests Failed:

Tests completed Failed Passed Skipped
6375 177 6198 452
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.TestBotWithoutRequest object at 0x0000013990675270>
bot_class = <class 'telegram._bot.Bot'>
bot_method_name = 'get_forum_topic_icon_stickers'
bot_method = <bound method ExtBot.get_forum_topic_icon_stickers of PytestExtBot[token=5737018356:AAH138SuiKQF0LDCWsfgWeXfjJ5d63kCWLA]>
offline_bot = PytestExtBot[token=5737018356:AAH138SuiKQF0LDCWsfgWeXfjJ5d63kCWLA]
raw_bot = PytestBot[token=5737018356:AAH138SuiKQF0LDCWsfgWeXfjJ5d63kCWLA]

    @bot_methods(ext_bot=False)
    async def test_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.bot if bot_method_name.lower().replace("_", "") == "getme" else None
        )
    
        # Check that ExtBot does the right thing
        bot_method = getattr(offline_bot, bot_method_name)
        raw_bot_method = getattr(raw_bot, bot_method_name)
>       assert await check_defaults_handling(bot_method, offline_bot, return_value=return_value)

tests\test_bot.py:500: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
tests\auxil\bot_method_checks.py:668: in check_defaults_handling
    raise exc
tests\auxil\bot_method_checks.py:636: in check_defaults_handling
    assert await method(**kwargs) in expected_return_values
telegram\ext\_extbot.py:1913: in get_forum_topic_icon_stickers
    return await super().get_forum_topic_icon_stickers(
telegram\_bot.py:8328: in get_forum_topic_icon_stickers
    return Sticker.de_list(result, self)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

cls = <class 'telegram._files.sticker.Sticker'>, data = None
bot = PytestExtBot[token=5737018356:AAH138SuiKQF0LDCWsfgWeXfjJ5d63kCWLA]

    @classmethod
    def de_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.
    
        """
>       return tuple(obj for obj in (cls.de_json(d, bot) for d in data))
E       TypeError: 'NoneType' object is not iterable

telegram\_telegramobject.py:457: TypeError
tests.test_bot.TestBotWithoutRequest::test_defaults_handling[Bot.get_chat]
Stack Traces | 0.003s run time
self = <tests.test_bot.TestBotWithoutRequest object at 0x0000013990674FF0>
bot_class = <class 'telegram._bot.Bot'>, bot_method_name = 'get_chat'
bot_method = <bound method ExtBot.get_chat of PytestExtBot[token=5737018356:AAH138SuiKQF0LDCWsfgWeXfjJ5d63kCWLA]>
offline_bot = PytestExtBot[token=5737018356:AAH138SuiKQF0LDCWsfgWeXfjJ5d63kCWLA]
raw_bot = PytestBot[token=5737018356:AAH138SuiKQF0LDCWsfgWeXfjJ5d63kCWLA]

    @bot_methods(ext_bot=False)
    async def test_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.bot if bot_method_name.lower().replace("_", "") == "getme" else None
        )
    
        # Check that ExtBot does the right thing
        bot_method = getattr(offline_bot, bot_method_name)
        raw_bot_method = getattr(raw_bot, bot_method_name)
>       assert await check_defaults_handling(bot_method, offline_bot, return_value=return_value)

tests\test_bot.py:500: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
tests\auxil\bot_method_checks.py:668: in check_defaults_handling
    raise exc
tests\auxil\bot_method_checks.py:636: in check_defaults_handling
    assert await method(**kwargs) in expected_return_values
telegram\ext\_extbot.py:889: in get_chat
    result = await super().get_chat(
telegram\_bot.py:4732: in get_chat
    return ChatFullInfo.de_json(result, self)
telegram\_chatfullinfo.py:517: in de_json
    data = cls._parse_data(data)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

data = None

    @staticmethod
    def _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.
        """
>       return data.copy()
E       AttributeError: 'NoneType' object has no attribute '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.TestBotWithoutRequest object at 0x0000013990675590>
bot_class = <class 'telegram._bot.Bot'>
bot_method_name = 'get_star_transactions'
bot_method = <bound method ExtBot.get_star_transactions of PytestExtBot[token=5737018356:AAH138SuiKQF0LDCWsfgWeXfjJ5d63kCWLA]>
offline_bot = PytestExtBot[token=5737018356:AAH138SuiKQF0LDCWsfgWeXfjJ5d63kCWLA]
raw_bot = PytestBot[token=5737018356:AAH138SuiKQF0LDCWsfgWeXfjJ5d63kCWLA]

    @bot_methods(ext_bot=False)
    async def test_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.bot if bot_method_name.lower().replace("_", "") == "getme" else None
        )
    
        # Check that ExtBot does the right thing
        bot_method = getattr(offline_bot, bot_method_name)
        raw_bot_method = getattr(raw_bot, bot_method_name)
>       assert await check_defaults_handling(bot_method, offline_bot, return_value=return_value)

tests\test_bot.py:500: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
tests\auxil\bot_method_checks.py:668: in check_defaults_handling
    raise exc
tests\auxil\bot_method_checks.py:636: in check_defaults_handling
    assert await method(**kwargs) in expected_return_values
telegram\ext\_extbot.py:4309: in get_star_transactions
    return await super().get_star_transactions(
telegram\_bot.py:9405: in get_star_transactions
    return StarTransactions.de_json(
telegram\_payment\stars\startransactions.py:159: in de_json
    data = cls._parse_data(data)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

data = None

    @staticmethod
    def _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.
        """
>       return data.copy()
E       AttributeError: 'NoneType' object has no attribute 'copy'

telegram\_telegramobject.py:385: AttributeError

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Bibo-Joshi added 6 commits December 29, 2024 22:00
@Bibo-Joshi
Get a Number of Tests passing
04de3ef
@Bibo-Joshi
Get defaults handling tests to run
ae639fb
@Bibo-Joshi
Use de_list instead of de_list_wo for required args
3fe033d
@Bibo-Joshi
Use de_json instead of de_json_wo for required args
c5f3bf7
@Bibo-Joshi
Fix failing tests
0a1b976
@Bibo-Joshi
More failures & one Improvement in TO
56cbe65
@Bibo-Joshi

Bibo-Joshi commented Dec 30, 2024

Copy link
Copy Markdown
Member Author

Admittedly, this turned out to be a bigger change than expected. Not really the removal of if not data: return None itself, but all that depended on that. However, this shows how much the internal logic of de_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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Bibo-Joshi
Bibo-Joshi marked this pull request as ready for review December 30, 2024 14:42
@Bibo-Joshi
remove type checking protocol from runtime
7c00b03
@Bibo-Joshi
Bibo-Joshi requested review from Poolitzer and harshil21 December 30, 2024 14:57
harshil21
harshil21 reviewed Jan 2, 2025
View reviewed changes

@harshil21 harshil21 left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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-

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread telegram/_business.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread telegram/_utils/argumentparsing.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread telegram/_business.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread tests/auxil/bot_method_checks.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@Bibo-Joshi Bibo-Joshi mentioned this pull request Jan 2, 2025
Extend & Unify Deserialization Testing #4635
Open
@Bibo-Joshi Bibo-Joshi added the 📋 do-not-merge-yet work status: do-not-merge-yet label Jan 2, 2025
Bibo-Joshi added 6 commits January 3, 2025 13:11
@Bibo-Joshi
Merge branch 'master' into de-json-empty-input
9682a90
# Conflicts:
#	telegram/_gifts.py
#	tests/auxil/bot_method_checks.py
@Bibo-Joshi
Revert "Use de_json instead of de_json_wo for required args"
cddf8c4
This reverts commit c5f3bf7
@Bibo-Joshi
Revert "Use de_list instead of de_list_wo for required args"
4a5a93d
This reverts commit 3fe033d
@Bibo-Joshi
Use data.get in remaining places
4c9ea41
@Bibo-Joshi
Rename functions according to review
e991b93
@Bibo-Joshi
Remove debug code
54b9988
@Bibo-Joshi
Bibo-Joshi requested a review from harshil21 January 3, 2025 12:58
@Bibo-Joshi Bibo-Joshi removed the 📋 do-not-merge-yet work status: do-not-merge-yet label Jan 5, 2025
harshil21
harshil21 approved these changes Jan 13, 2025
View reviewed changes

@harshil21 harshil21 left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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:

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread telegram/_giveaway.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@Bibo-Joshi
Merge branch 'master' into de-json-empty-input
d942921
@Bibo-Joshi
Review
05f52fa
@Bibo-Joshi
Bibo-Joshi merged commit dd592cd into master Jan 14, 2025
@Bibo-Joshi
Bibo-Joshi deleted the de-json-empty-input branch January 14, 2025 16:12
@github-actions github-actions Bot locked and limited conversation to collaborators Jan 22, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@harshil21 harshil21 harshil21 approved these changes
@Poolitzer Poolitzer Awaiting requested review from Poolitzer

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

🛠 refactor change type: refactor

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Simplify Handling of empty data in TO.de_json

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@Bibo-Joshi @harshil21
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.