Skip to content
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

Feature Request: filter_* => on_* #766

Open
Hirrolot opened this issue Nov 16, 2022 · 4 comments
Open

Feature Request: filter_* => on_* #766

Hirrolot opened this issue Nov 16, 2022 · 4 comments
Assignees
Labels
A-update-managment Area: update managment (dptree & co) K-feature-request Kind: request for implementing a feature S-blocked Status: this is blocked on something

Comments

@Hirrolot
Copy link
Collaborator

Currently, we favour filter_* naming, such as Update::filter_message or filter_command or whatever. I suggest changing that to Update::on_message, on_command, etc.

(However, we should not change dptree::filter and its siblings, since dptree has less context than teloxide in general.)

Pros

  • It's shorter.
  • It's generally more adopted across many other libraries and frameworks.

Cons

  • Deprecation needed.

Alternatives

  • Don't do anything.
@Hirrolot Hirrolot added the K-feature-request Kind: request for implementing a feature label Nov 16, 2022
@WaffleLapkin
Copy link
Member

I am not sure how I feel about this. On one hand, on_* does sound nice, but I'm afraid that it will lead to more confusion of that it actually does.

For example for filter_command we currently require that Message is in the context, so you need to filter_message().branch(filter_command().<...>). With on_* this looks more unintuitive, IMO.

@Hirrolot
Copy link
Collaborator Author

I don't know, as for me, filter_message().branch(filter_command()) is just the same as on_message().branch(on_command()). filter may be confusing too, since it doesn't explicitly state what dependencies are needed, does it?

@WaffleLapkin
Copy link
Member

For me "on_x" feels more like handlers[x] = f i.e. execute function every time a particular event is being handled. But what our function actually does is change what update kinds are handled forward (and also change the context). So uhhh, I'm not sure it's a good fit.

filter may be confusing too, since it doesn't explicitly state what dependencies are needed, does it?

Yes, of course. Ideally ig we would check this at compile time, or at least have better docs that clearly highlight provide/require relationships between all the dispatch functions.

@Hirrolot
Copy link
Collaborator Author

Ok, let me think about that a bit. I'm currently working on some dptree PR that might make things simpler, and then will come back to the discussion.

@Hirrolot Hirrolot added the S-blocked Status: this is blocked on something label Nov 16, 2022
@WaffleLapkin WaffleLapkin added the A-update-managment Area: update managment (dptree & co) label Jan 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-update-managment Area: update managment (dptree & co) K-feature-request Kind: request for implementing a feature S-blocked Status: this is blocked on something
Projects
None yet
Development

No branches or pull requests

2 participants