nerzh / swift-telegram-sdk

🤖 The wrapper for the Telegram Bot API written in Swift. It's not a framework. There is no special syntax here. This is a library that implements all Telegram Bot API methods, which is available to you to work with Vapor, Smoke, Hummingbird, FlyingFox.
https://core.telegram.org/bots/api#available-methods
MIT License
203 stars 32 forks source link

Swift Concurrency + new syntax #8

Closed GiacomoLeopizzi closed 1 year ago

GiacomoLeopizzi commented 1 year ago

Dear Maintainers,

As you can see from the README examples, I'm sending a pull request that introduces the following:

  1. Better integration with Vapor by accessing the bot from the Application object (using app.telegram).
  2. The handler's callback is now marked as "async" to use the new Swift Concurrency API.
  3. New syntax to write handlers (previous one is still fully working).

I would personally suggest, if you accept my pull request, to start a new major release (2.0.0), considering that because of point 1, the code is not compatible with the previous version.

Kind regards, Giacomo

nerzh commented 1 year ago

Thanks, I need some time to check all the changes. I will do this during off hours

GiacomoLeopizzi commented 1 year ago

@nerzh No problem! In my fork I made other two changes that perhaps might be interesting for you:

GiacomoLeopizzi commented 1 year ago

Also please decide about the TGHandlerCallback. Temporarily, I defined it as:

public typealias TGHandlerCallback = (_ update: TGUpdate, _ bot: TGBotPrtcl) async throws -> Void

This way the callback might throw an error that technically is not handled (it is just printed to console), as the following snippets shows.

public func handle(update: TGUpdate, bot: TGBotPrtcl) async {
        do {
            try await callback(update, bot)
        } catch {
            TGBot.log.error(error.logMessage)
        }
 }

Two possible solutions, might be:

  1. Defining TGHandlerCallback as

    public typealias TGHandlerCallback = (_ update: TGUpdate, _ bot: TGBotPrtcl) async -> Void

    forcing the developers to handle throwing functions inside their definition of TGHandlerCallback.

  2. Leaving TGHandlerCallback as it is, and adding a variable to TGBotPrtcl to handle all the errors that are not handled in the callback. The new variable might be defined as:

    var onError: ((Error) -> Void)? { get set }
nerzh commented 1 year ago

Hello, there are a lot of changes in your pull request with which i agree and some i don't. If they were parts, then I would gladly accept some of them. Also, in your pull request, not all parts of the app were migrated to the new concurrency model. I really appreciate your contribution, but unfortunately I can not accept it. I updated the library a bit and added what users asked me about. I followed your advice and moved the logic to the NIO event loop for the dispatcher #9. If you make contributions in the future, please do it in parts, as it is easier to accept and, most importantly, it is easier to understand changes.