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

High-level command handlers #25

Closed
wants to merge 7 commits into from
Closed

High-level command handlers #25

wants to merge 7 commits into from

Conversation

tucnak
Copy link
Owner

@tucnak tucnak commented Nov 20, 2015

Current implementation prototype is quite dumb. All it does is primitive routing:

bot, err := telebot.NewBot(os.Getenv("TELEBOT_SECRET"))
if err != nil {
    log.Println(err)
    return
}

bot.Handle(telebot.Default, func(_ telebot.Context) {
    // Default handler
})

bot.Handle("/fun", func(_ telebot.Context) {
    // Would get executed on /fun <whatever=0>
})

bot.Handle("/evil", func(_ telebot.Context) {
    // Would get executed on /evil <whatever=0>
})

bot.Serve()

I still think that we should provide some handy API for "user interactipn scripts". E.g. abstract away from actual command handling and get along with "request-details-answer" sort of scripts.

@tucnak tucnak self-assigned this Nov 20, 2015
@tucnak
Copy link
Owner Author

tucnak commented Nov 20, 2015

I definitely need some help and advice here... @Ronmi @ejamesc @olebedev

@Ronmi
Copy link
Contributor

Ronmi commented Nov 24, 2015

Nice point of view!

Does user interaction means something like A system status monitoring bot you can send it commands to get cpu loading/memory usage or A bot provides some commands to subscribe/unsubscribe to some RSS feeds? If so, we might want to discuss about how command argument are transfered: in the /cmd arg1 arg2 arg3 format or something like botfather does?

I personally think botfather approach has better UX, but it could be too complex to implement in bot framework: the number of arguments and the type of each argument varies. It should be part of business logic.

The /cmd args format is restricted, but I think it would work for simple bot like the two examples above. And it would be great help if bot framework can parse the arguments for me, like: (surely the args and msg below can be integrated into context)

bot.Handle("/cmd", func(args []string, msg Message) {})

And how about photo/video/other-type messages? These messages seems not able to share same structure/mechanism with text messages.

@olebedev
Copy link

@Ronmi I think argument's parsing is not related for this issue.
As I see @tucnak hide some bot intial logic into .Serve(), it seem that some of initial arguments need to be explicitly transferred. For example:

bot.Serve(5e9 /* timeout time.Duration */)

Also middlewares may be useful in these cases, IMO. Example:

bot.Use("/fun", mdllwr)
// or
bot.Handle("/fun", mdllwr, func(_ *Context){ /* do stuff */ })

As for the rest it looks good to me.

@tucnak
Copy link
Owner Author

tucnak commented Nov 24, 2015

@Ronmi I don't really think that argument parsing should be part of the library (maybeh later), cos it's quite unique to the end bot logic: e.g. do you wanna split strings by spaces or you don't? In the end there are loads of different message types, thus we shouldn't really focus on texts only.

By user interaction I mean some sort of a plot, maybe in a tree-like form, which allows to build sophisticated interaction pipelines, so you don't need to do that with if's on your own. Here is a dreamy API that just came out of my head:

bot.Start(startHandler)
    .Get(telebot.Text, usernameHandler, errHandler)
    .Get(telebot.Picture, profilePicHandler, errHandler)
    .Finish(sayThanksHandler)
    .Perform()

So this pipeline is capable of handling "/start", asking for a username, profile picture, saying thanks after all. Again, just a trivial example out of my mind.

@olebedev Yeah, we definitely should reveal timeout to public. I am still not sure we are already here to provide middleware sort of stuff :)

@Ronmi
Copy link
Contributor

Ronmi commented Nov 25, 2015

I have to say sorry to you two because I'm not quite know how to express well in English. And to @tucnak specially, because I misunderstand your point.

I'm not saying about to parse argument, through it would be a plus to me. I'm talking about transferring arguments. What @tucnak described in latest comment is exactly which I refer as botfather approach in my last comment. I am really sorry about I did not express it clearly.

My botgoram is also this approach, but it's mainly for my work, which will generate basic program structure from UML diagram, not for hand-writing. As I wrote botgoram, I found that I have to have a mechanism for storing user states: what command and which step users are in (possibly also the data from previous step), or bot will not able to know which handler to call. Store state data in memory might not be best practice. Users might test the bot, find it not fun or not suitable for his work, then leave and forget the bot. This can cause many trash in memory, and memory is expansive. So I think an interface to let user provide their state storage implementation is critical in this approach.

API looks good to me, excepts lacking of "tree-form" for supporting multiple commands. I have an idea, how about a new type Action?

acts := []Action{Action{}}
acts[0].Get(telebot.Text, usernameHandler, errHandler)
    .Get(telebot.Picture, profilePicHandler, errHandler)
    .Finish(sayThanksHandler)
bot.Perform(acts)

So we can use only two integers to determine user state.

BTW, I found that travis-ci.org can set environment variables in settings tab, maybe can help for removing secret token from .travis.yml file?

@tucnak
Copy link
Owner Author

tucnak commented Nov 25, 2015

BTW, I found that travis-ci.org can set environment variables in settings tab, maybe can help for removing secret token from .travis.yml file?

Nah, wouldn't work for forks then. I am totally fine with this bot being used for testing, I hope people won't become impudent.

rodrigodiez added a commit to rodrigodiez/telebot that referenced this pull request Apr 3, 2016
@JJ
Copy link
Contributor

JJ commented Sep 24, 2016

👍 just to keep tabs on this issue.

@tucnak
Copy link
Owner Author

tucnak commented Nov 10, 2016

  • This doesn't play with neither queries nor callback at all
  • No default handling
  • I'm afraid current tip doesn't work at all

Lot's of work to do... 🎱

@tucnak tucnak closed this Aug 15, 2017
@tucnak tucnak deleted the handlers branch August 15, 2017 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants