PaulSonOfLars / gotgbot

Autogenerated Go wrapper for the telegram API. Inspired by the python-telegram-bot library.
MIT License
509 stars 115 forks source link

Update to Bot API 5.3 #26

Closed PaulSonOfLars closed 3 years ago

PaulSonOfLars commented 3 years ago

What

Update library to Bot API 5.3.

Breaking changes:

Code generation changes:

Impact

PaulSonOfLars commented 3 years ago

New structs in telegram's API update cause some issues with regards to handling incoming updates.

Previously, "varying" types were only ever sent TO the API. They were never received FROM the API. (example: InlineQueryResult). This makes it easy to use interfaces, since developers rarely needs to interact with them once theyre instantiated.

As of 5.3, Telegram has separated the ChatMember type into multiple sections. This type is now "received" by telegram. This makes it a new issue since it needs to be accessed by users too. We are now at a crossroads where we should choose between the "correct" way (copying the telegram API), or the "usable" way (simplifying usage).

Option 1: Following the td spec to the letter.

An interface should be created, which wraps each of the different new ChatMember structs. This makes receiving ChatMember updates much harder, as custom unmarshalling is required to parse the event. Once this is done, extra processing is required to assert that the interface is a usable struct{}. This type assertion is error prone and can lead to panics if not handled properly.

Example (user) code:

type ChatMember interface {
  Type() string
  // ... other unexported methods
}

type ChatMemberAdministrator struct {
 // ... all fields from docs
}

func handleChatMember(b *gotgbot.Bot, ctx *ext.Context) error {
  cm := ctx.ChatMember.NewChatMember // of type ChatMember interface 
  // cm.User wont compile, since `User` is not accessible from the interface
  if cm.Type() == "administrator" {
    admin, ok := cm.(ChatMemberAdministrator) // This new step is required for EACH type to assert it is the correct struct
    if !ok {
      // This shouldnt fail. But if it does, itll panic unless you have the boolean check.
    }
    admin.User // this works fine, since this is now a legitimate struct with exposed fields
  }
}

Option 2: Merge all the ChatMember types into one

This essentially retains the current functionality observable in v10. It creates a diversion from the docs which may cause confusion in the future. Since we no longer match telegram, the docs also wont be accurate anymore, so the doc generation will need altering to specify which fields are optional and only appear for certain fields

Personally, I have a strong preference for Option 2. Not needing to cast and deal with interfaces seems like a big plus to me. Overall, this change seems like extra confusion from telegram, and being able to simplify that for our use-case is a win in my books.

jon4hz commented 3 years ago

What about building a structure like this:

type ChatMemberOwner struct {
    // ... all fields from docs
}

type ChatMember struct {
    Owner *ChatMemberOwner
}

This would allow to check if a field is nil in the ChatMember struct.

And as discussed in the telegram group you could add a helper function to get the user (e.g.) that the developers wont have to check all six fields for that value every time.

PaulSonOfLars commented 3 years ago

After discussion in the telegram support chat, it was agreed upon that retaining the telegram types, but adding a Merge() method to create a "merged" version of that type (which has all the fields of the parent types) would be the best approach. This has been reflected in the latest commits; let me know how it works.