Skip to content

Conversation

@Bibo-Joshi
Copy link
Member

Closes #2885. supersedes & closes #2890

in hindsight the review process of #2890 revealed to me that it might be better to

  • properly split PrefixHandler from CommandHandler
  • not have the commends updatable via setters. For now I completely removed that. If users actually use that & ask for it, we cann add proper method update_commands() etc. IMO

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

@Bibo-Joshi Bibo-Joshi added 🛠 refactor change type: refactor 🛠 breaking change type: breaking labels May 18, 2022
@Bibo-Joshi Bibo-Joshi added this to the v20.0 milestone May 18, 2022
@Bibo-Joshi Bibo-Joshi requested review from Poolitzer and harshil21 May 18, 2022 15:57
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.

Question:

Comment on lines +75 to +77
Warning:
When setting :paramref:`block` to :obj:`False`, you cannot rely on adding custom
attributes to :class:`telegram.ext.CallbackContext`. See its docs for more info.
Copy link
Member

Choose a reason for hiding this comment

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

hm, what is this warning doing here? Is it saying that the custom .args attribute which this handler adds wont be there?

Copy link
Member Author

Choose a reason for hiding this comment

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

When you have handlers in multiple groups and you do context.custom_arg = foo in a lower group, you cannot rely on context.custom_arg being available in the higher group. We have this warning in all handlers.

Copy link
Member

@harshil21 harshil21 left a comment

Choose a reason for hiding this comment

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

tests should be updated + .rst file renaming needed

@Bibo-Joshi Bibo-Joshi changed the title Start pulling apart CH & PH Split CH & PH May 18, 2022
Copy link
Member

@harshil21 harshil21 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 nitpick

Copy link
Member

@harshil21 harshil21 left a comment

Choose a reason for hiding this comment

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

Lgtm

@Bibo-Joshi Bibo-Joshi merged commit dc13b69 into master May 26, 2022
@Bibo-Joshi Bibo-Joshi deleted the prefix-command-handler branch May 26, 2022 09:10
@github-actions github-actions bot locked and limited conversation to collaborators Jun 2, 2022
@harshil21 harshil21 modified the milestones: v20.0, v20.0a1 Sep 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

🛠 breaking change type: breaking 🛠 refactor change type: refactor

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Rename CommandHandler.command to CH.commands (plural) …

4 participants