Skip to content

CallbackData with default prefix #1668

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

Draft
wants to merge 3 commits into
base: dev-3.x
Choose a base branch
from

Conversation

Coderik
Copy link

@Coderik Coderik commented Apr 5, 2025

Description

This PR modifies CallbackData and makes the prefix optional. By default the class name is used as prefix.

This makes it less tedious to define a large number of custom CallbackData classes. For instance:

class SettingsActions:
    class GoBack(CallbackData):
        pass

    class ShowFirstMenu(CallbackData):
        pass

    class ShowSecondMenu(CallbackData):
        pass

    class SetSomeFlag(CallbackData):
        flag: bool

NOTE: This PR does not touch the documentation, because I was not sure how welcome this change would be. It made my own code easier to maintain. If you think it's useful, I'll update the documentation.

Type of change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

How Has This Been Tested?

This PR removes one test that checks that an exception is raised when the prefix is not provided. And adds one test that checks the new behavior.

  • Test that the prefix is set to class name, when not given explicitly
  • Test that the prefix can be set explicitly

Test Configuration:

  • Operating System: Linux
  • Python version: 3.10.12

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@github-actions github-actions bot added the 3.x Issue or PR for stable 3.x version label Apr 5, 2025
Copy link

github-actions bot commented Apr 5, 2025

✔️ Changelog found.

Thank you for adding a description of the changes

cls.__separator__ = kwargs.pop("sep", ":")
cls.__prefix__ = kwargs.pop("prefix")
cls.__prefix__ = prefix
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not simply do this?
cls.__prefix__ = kwargs.pop("prefix", cls.__name__)

@JrooTJunior JrooTJunior requested a review from Copilot April 6, 2025 11:41
Copilot

This comment was marked as off-topic.

@JrooTJunior
Copy link
Member

Please provide more details and explain why these changes are needed.

Please note that callback data has a strict size limit (64 bytes) on the Telegram Bot API side, so you should not increase the data size. In most cases, 1-5 characters are enough for the prefix part.

But on the other hand, class names should be as clear and self-descriptive as possible, so in most cases this means that there should be more than 5 characters to explain what this class does (Exactly what you showed in your example).

Therefore, these statements are mutually exclusive.

@Coderik
Copy link
Author

Coderik commented Apr 10, 2025

Please provide more details and explain why these changes are needed.

My use case is a fairly large, a couple of levels deep settings menu, built on inline keyboards. I use CallbackData classes as actions to move between levels and adjust settings. In this case three are many CallbackData classes either without, or with a single data field. So, there is no issues with the 64 bytes limit. But I'm forced to define the prefix anyways.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.x Issue or PR for stable 3.x version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants