Skip to content

ChM.CREATOR -> Owner & use enums for dynamic types #2817

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Apr 30, 2022
Merged

ChM.CREATOR -> Owner & use enums for dynamic types #2817

merged 6 commits into from
Apr 30, 2022

Conversation

Bibo-Joshi
Copy link
Member

@Bibo-Joshi Bibo-Joshi commented Dec 13, 2021 •
edited by harshil21
Loading

  • renames ChatMember.CREATOR to OWNER since for some reason that's what TG named it even though the type is 'creator' …
  • and also ChatMember.KICKED -> ChatMember.BANNED
  • for classes where we didn't hard code the type, try to resolve the type to the appropriate enum

Checklist for PRs

  • Added .. versionadded:: version, .. versionchanged:: version or .. deprecated:: version to the docstrings for user facing changes (for methods/class descriptions, arguments and attributes)
  • Created new or adapted existing unit tests
  • Added myself alphabetically to AUTHORS.rst (optional)
  • Added new classes & modules to the docs

If the PR contains API changes (otherwise, you can delete this passage)

  • New classes:

    • Added self._id_attrs and corresponding documentation
    • __init__ accepts **_kwargs

  • Added new shortcuts:

    • In Chat & User for all methods that accept chat/user_id
    • In Message for all methods that accept chat_id and message_id
    • For new Message shortcuts: Added quote argument if methods accepts reply_to_message_id
    • In CallbackQuery for all methods that accept either chat_id and message_id or inline_message_id

  • If relevant:

    • Added new constants at telegram.constants and shortcuts to them as class variables
    • Added new handlers for new update types
    • Added new filters for new message (sub)types
    • Added or updated documentation for the changed class(es) and/or method(s)
    • Updated the Bot API version number in all places: README.rst and README_RAW.rst (including the badge), as well as telegram.constants.BOT_API_VERSION
    • Added logic for arbitrary callback data in tg.ext.Bot for new methods that either accept a reply_markup in some form or have a return type that is/contains telegram.Message

@Bibo-Joshi Bibo-Joshi added enhancement 🛠 breaking change type: breaking labels Dec 13, 2021
@Bibo-Joshi Bibo-Joshi added this to the v14 milestone Dec 13, 2021
@Bibo-Joshi Bibo-Joshi requested a review from harshil21 December 13, 2021 20:45
github-actions[bot]
github-actions bot reviewed Dec 13, 2021
View reviewed changes
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Hey there. Relax, I am just a little warning for the maintainers to release directly after merging your PR, otherwise we have broken examples and people might get confused :)

@Bibo-Joshi
Copy link
Member Author

The tests fail, because:

  • enum members have an attribute __objclass__ which contains class such that inspect works correctly - see https://bugs.python.org/issue19263
  • BP.replace/insert_bot doesn't handle types, so a warning is raised and the test fails

Note that the warning will also be issued when persisting other objects that use the new enum constants, e.g. InlineQueryResult* or Poll - it just didn't show yet. Having a warning raised by the classes PTB ships seems subobtimal. But so does special casing for enums … I see different options here:

  • we're okay with the warning and go ahead with this
  • we're not okay with the warning and don't use the enum members as constants
  • we're not okay with the warning and add special casing for enums to replace/insert
  • we postpone until we've had a more general discussion on replace/insert_bot

TBH I'm somewhat in favor of the last 😄

harshil21
harshil21 approved these changes Dec 14, 2021
View reviewed changes
Copy link
Member

@harshil21 harshil21 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Changes look fine. Do you want to apply this to the other enums like BotCommandScope too?

TBH I'm somewhat in favor of the last 😄

yep, we can we defer this for now.

@Bibo-Joshi
Copy link
Member Author

For those types of classes we have hard-coded the type, e.g.

python-telegram-bot/telegram/_botcommandscope.py

Line 174 in 74c4270

super().__init__(type=BotCommandScope.ALL_CHAT_ADMINISTRATORS)

Poll, Chat and MessageEntity were the only classes with a type attribute that don't have a subclass for every type …

Poolitzer
Poolitzer approved these changes Dec 15, 2021
View reviewed changes
Copy link
Member

@Poolitzer Poolitzer left a comment

Choose a reason for hiding this comment

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

LGTM, +1 for defering, its not important right now imo

@harshil21 harshil21 added the 📋 do-not-merge-yet work status: do-not-merge-yet label Jan 6, 2022
@harshil21 harshil21 removed the 📋 do-not-merge-yet work status: do-not-merge-yet label Apr 26, 2022
@harshil21 harshil21 mentioned this pull request Apr 27, 2022
Merged
4 tasks
@harshil21 harshil21 merged commit 2abe90d into v14 Apr 30, 2022
@Bibo-Joshi Bibo-Joshi deleted the more-enums branch May 2, 2022 06:11
Bibo-Joshi added a commit that referenced this pull request May 6, 2022
@Bibo-Joshi
Use enums for dynamic types & rename two attributes in ChatMember (#2817
823d030 
)
@github-actions github-actions bot locked and limited conversation to collaborators May 9, 2022
@Bibo-Joshi Bibo-Joshi added 🔌 enhancement pr description: enhancement and removed enhancement labels Nov 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

@Poolitzer Poolitzer Poolitzer approved these changes

@harshil21 harshil21 harshil21 approved these changes

Assignees
No one assigned
Labels
🛠 breaking change type: breaking 🔌 enhancement pr description: enhancement
Projects
None yet
Milestone
v20.0a0
Development

Successfully merging this pull request may close these issues.
3 participants
@Bibo-Joshi @Poolitzer @harshil21