refactor : inline-query handler type and register - #294
Conversation
negasus
left a comment
There was a problem hiding this comment.
Thanks for picking up #293 — the idea is right and the example does read better afterwards. One thing in the implementation needs to change before I can take it, plus a design question I would like your view on.
1. The new case breaks the contract of match().
Every other case sets data (and entities where they exist) and then lets the shared matching logic below decide. The new one short-circuits:
case HandlerTypeInlineQuery:
if update.InlineQuery == nil {
return false
}
return trueHandlerTypeInlineQuery is an exported constant, so it can be passed to the exported RegisterHandler(handlerType, pattern, matchType, ...). With this code, RegisterHandler(HandlerTypeInlineQuery, "foo", MatchTypeExact, h) silently matches every inline query, because the pattern is never consulted. RegisterHandlerRegexp has the same problem. A registration that quietly ignores half its arguments is a trap.
Please make it behave like the others:
case HandlerTypeInlineQuery:
if update.InlineQuery == nil {
return false
}
data = update.InlineQuery.Queryand have RegisterInlineQueryHandler register with an empty pattern and MatchTypePrefix — an empty prefix matches everything, so the convenience helper keeps its "handle all inline queries" behaviour. As a bonus, users then get matching on the inline query text for free, which is a common thing to want.
2. Please add tests.
handlers_test.go covers the existing cases and this branch is untested. It matters more than usual here: #222 is in flight specifically to bring handlers.go to full coverage, and these two will also conflict textually, so whichever lands second needs a rebase.
3. Design question, and this is on me rather than you.
The same gap exists for chosen_inline_result, shipping_query, pre_checkout_query, poll_answer, my_chat_member and others. Adding one bespoke constant plus one bespoke registration method for inline queries sets a precedent I would have to follow for all of them. Note that RegisterHandlerMatchFunc already covers the case today:
b.RegisterHandlerMatchFunc(func(u *models.Update) bool { return u.InlineQuery != nil }, handler)so this is convenience rather than a missing capability. I am inclined to take it anyway, since inline queries are common enough to deserve the shortcut — but I would like to hear whether you see this as the first of a series or as a one-off.
Naming: the neighbours are RegisterHandlerRegexp and RegisterHandlerMatchFunc, so RegisterHandlerInlineQuery would fit the existing scheme better than RegisterInlineQueryHandler.
Nits: there is a stray blank line at the end of the new switch case; and in the example, opts := []bot.Option{} followed by bot.New(token, opts...) can just be bot.New(token).
Description
This PR addresses issue #293 by refactoring the inline query handling mechanism to make it cleaner and more idiomatic.
Instead of relying on the existing workaround in the examples directory, this change introduces a dedicated handler type specifically designed for inline query interactions.
Changes Made
HandlerTypeInlineQueryto theHandlerTypeenum/constants.matchfunction logic with a newcaseto properly routeHandlerTypeInlineQuery.examples/to demonstrate the new implementation.Related Issue
#293