tucnak / telebot

Telebot is a Telegram bot framework in Go.
MIT License
3.97k stars 465 forks source link

V2: Moving towards better API #93

Closed tucnak closed 6 years ago

tucnak commented 7 years ago

I set the goal to release v2 by the end of September (see updated 2.0 milestone). As it's going to be a pretty major release, I suggest re-thinking some of the API. Our biggest competitor, telegram-bot-api, 625 stars, is still superior to Telebot in terms of API, so I believe in order to stand out we must provide additional and moreover, sensible value. Telebot has always positioned itself as the framework, while our competitors were and will remain an API wrapper.

Here are some of the issues I'd like to address (in the order of significance).

Memory efficiency

Message is the fattest type in Telebot, taking as much as 1544 bytes on my system (~1.5KB). Mainly because it stores lots of optional fields by-value. This is clearly borderline stupid and uncomfortable (you can't test optionals against nil) and I can't recall why I'd ever thought it was a good idea.

I trivially managed (https://github.com/tucnak/telebot/commit/d8b3194888f53bba616c37301d67adb8035944bb) to reduce memory usage of Message down to 504 bytes (~0.5KB), making a 67.4% decrease in memory usage, which is pretty fucking good, considering Message is the most-used object. No more hassle around optionals also.

Use of reflection for prettier API

We currently use no reflection at all, which is probably a good thing. That said, I think introducing some light weight interface{}-kind of reflection for "optional" parameters may be a good thing.

SendMessage() / Send()

// Definition
func (b *Bot) SendMessage(r Recipient, message string, options ...interface{}) error {}

// Use
b.SendMessage(user, "message") // <-- much better, isn't it?
b.SendMessage(user, "message", &SendOptions{})

In fact, we could go further and generalize it all the way down to Send():

// Definition
func (b *Bot) Send(r Recipient, things ...interface{}) error {}

// Use
b.Send(user, "message")
b.Send(user, "message", &SendOptions{})
b.Send(user, photo)
// Etc..

I wonder whether it'll damage Telebot's performance much, we can't really tell without proper benchmarking. That said, considering there's not much high load in Telegram world, we still must end up the fastest bloke on the block by a huge margin (comparing w/ e.g. Python implementations).

Pros:

Cons:

Sendable

In fact, we could avoid losing some of the safety and a fair chunk of casting and still introduce a better API by introducing a Sendable interface for all the sendable objects like Photo, Audio, Sticker, etc:

type Sendable interface {
    Send(*Bot) error
}

Each and every corresponding sendable type would implement the necessary actions (which quite differ from type to type). This could also be used as a part of a prettier Send(Recipient, ..interface{}) too.

Message builders

I thought about it for some time now and I believe we could benefit from some sort of builder API for sending messages, like this:

// Use
bot.V()
    .To(recipient)
    .Text("This is a text message")
//  .Photo(&photo) or any other sendable
    .Markdown()
    .Keyboard(keyboard)
    .Error(handlerFunc)
    .Send()

It looks so good!

Other...

I also thought about implementing things like a state machine (telebot users would need to specify a list of states their bot users can be in and also, specify relations between these states) and a global error handler. Unfortunately, I couldn't come up with a reasonable API for "plots" (a state machine) yet though. I usually panic on bot errors and handle them up the stack anyway, so classic if err != nil error handling is rather redundant. I'd love to do things like:

bot.ErrorHandler = func(err error) {
    panic(err)
}

bot.Handle("/start", func(c Context) {
    bot.Send(c.Message.Sender, "Hello") // calls ErrorHandler() -> panic()
})

/cc @zoni @Ronmi @ahmdrz

UPD: I also feel like we are ovelooking inline queries and callbacks from the API design perspective... Bot start/listen/serve is confusing also!

tucnak commented 7 years ago

/cc @irgendwr

irgendwr commented 7 years ago

I really like the Idea of using reflection as a way of simplifying the send methods! But wouldn't your proposed message builder mean that you have multiple ways of doing the same thing (ie. sending a message)? Might be confusing, idk. The error handler would also be very useful to reduce repetitive error checking.

start/listen/serve is confusing

As you can guess I like the "There should be one-- and preferably only one --obvious way to do it." mentality 😄

irgendwr commented 7 years ago

I haven't used callbacks yet but they could probably be handlers too, registered per user/state or per message for example.

tucnak commented 7 years ago

I think Serve is kind of useless because when I define a handler I expect it to be called by default.

I couldn't introduce it any other way without breaking backwards compat. 😞

Listen is weird as it's not really needed

Fair enough! I kept it for backwards compat too, even though we can now let it go in v2.

Couldn't Start also be merged with NewBot?

Good question. I don't think so. All the receiving channels must be initialized when long polling starts, so we probably should stick to the 3-phase init process: NewBot, channels & handlers, and Start.

I haven't used callbacks yet but they could probably be handlers too, registered per user or per message for example.

I use callbacks for better UI mostly: inline keyboard buttons cause message updates. What's the benefit of "registering" callbacks as handlers and wiring them to messages, especially considering that associated messages are returned as a part of a callback struct?

matteocontrini commented 7 years ago

Any progress? It seems there hasn't been much activity after August 25...

tucnak commented 6 years ago

@matteocontrini No progress at all, I'm still pretty much waiting for feedback.

tucnak commented 6 years ago

FYI, I just added Send(), Reply() and Forward() the three "generic" methods. All of them accept 2+ arguments (recipient, payload and "options"). Since both payload and options are of type interface{}, it lets you pass either string or Sendable as payload and pretty much anything for options.

But what exactly is an "option"? Nice thing about this API is that by "options" it doesn't necessarily mean *SendOptions. Both *SendOptions, *ReplyMarkup and one of the const Options (which I'm still working on, but basically these are just fancy shortcuts) will work, following arguments overriding preceding ones. Look:

b.Send(user, "Message")
b.Send(user, photo, tb.ForceReply)

b.Send(user, photo, tb.NoPreview, tb.ForceReply)
// instead of
b.Send(user, photo, &tb.SendOptions{
    DisableWebPagePreview: true,
    ReplyMarkup: &ReplyMarkup{ForceReply: true},
})

// You can override some properties of stored options also:
savedOptions := &tb.SendOptions{}

b.Send(user, "text", savedOptions, tb.NoPreview) // overrides DisableWebPagePreview from `savedOptions` 

Let me know if there's anything that doesn't feel right.

tucnak commented 6 years ago

Blimey! You know what?

It is.

Just.

Perfect.

I'm in overdrive, baby!!!

I don't even care anymore because I'm in the process of making Telebot God Emperor Bot Thing so hey boys and girls chill out