Smertig / banana

🍌 Modern C++ Telegram Bot API library
https://smertig.github.io/banana/master
MIT License
41 stars 4 forks source link

Add Boost.Beast agent with callback interface. #18

Closed UltraCoderRU closed 1 year ago

UltraCoderRU commented 1 year ago

This PR implements second part of #2 for Boost.Beast agent.

To disambiguate calls to API methods without real arguments (like log_out()) those methods now don't have empty args param. Technically, it is a breaking change, but only for users, who explicitly pass empty args to those methods (maybe, nobody).

I made this PR for personal project, but hope it will be helpful. :)

Smertig commented 1 year ago

Thank you for such a big PR! It's really cool that you changed docs, examples and codegen scripts. Callback interface should be useful, but I have some thoughts:

  1. It's not possible to ignore API result if banana::agent::beast_callback is used. One should always pass something like [](auto){ /* empty */ } as a callback, this may be quite annoying. However, I think there's a way to fix it: we should merge make_callback functionality into make_future_monadic. They have similar requirements (existance of do_async_request method in underlying agent) and one should be able to use both APIs at the same time with single agent. Thus, we don't have to need two different agents.
  2. It's non obvious for now whether it's possible to use callback API with the specific agent. It should be documented at least. We can also make such APIs either SFINAE-friendly or add static_asserts in generated APIs with detailed reason (something like "Your agent is incompatible with callback-based API").
  3. I don't know what to do with non-monadic version callback-based version. Maybe we should simply ignore this case for now?

It would be great, if you implement fix described in the first point (at least), so I'll accept this PR. If not, I can also accept it and make this changes myself.

P.S. I apologize for not responding for a long time (was on vacation)

UltraCoderRU commented 1 year ago
  1. It's not possible to ignore API result if banana::agent::beast_callback is used. One should always pass something like [](auto){ /* empty */ } as a callback, this may be quite annoying. However, I think there's a way to fix it: we should merge make_callback functionality into make_future_monadic. They have similar requirements (existance of do_async_request method in underlying agent) and one should be able to use both APIs at the same time with single agent. Thus, we don't have to need two different agents.

I agree: impossibility to ignore result is annoying. But I think, if we merge make_callback into make_future_monadic, the latter's name becomes confusing. It should be named make_async_monadic or somehow like that. We can create type alias for make_future_monadic to preserve backward compatibility.

  1. I don't know what to do with non-monadic version callback-based version. Maybe we should simply ignore this case for now?

Sorry, but I don't understand. How callback-based interface can be non-monadic?

Smertig commented 1 year ago

But I think, if we merge make_callback into make_future_monadic, the latter's name becomes confusing. It should be named make_async_monadic or somehow like that. We can create type alias for make_future_monadic to preserve backward compatibility.

Agreed. make_async_monadic is a good name. It may be confused with wrap_async, however the latter is more about std::async (that is almost useless). Can you please rename make_future[_monadic] and also beast_future[_monadic] into make_async[_monadic] & beast_async[_monadic] and keep type aliases with old names for backward compatibility?

Sorry, but I don't understand. How callback-based interface can be non-monadic?

I can't imagine it too, that's why I mentioned this as a problem :) It's mostly about symmetry. All agents have two versions: monadic (that returns expected<T>) and non-monadic (that throws an exception in case of an error). For callback-based API we can provide only monadic version, there's no way to get exception-behavior directly (other than calling .value() on expected). However, missing symmetry is not a big problem now, because we decided to merge beast_future_monadic and beast_callback into beast_async, so nevermind.

UltraCoderRU commented 1 year ago

Can you please rename make_future[_monadic] and also beast_future[_monadic] into make_async[_monadic] & beast_async[_monadic] and keep type aliases with old names for backward compatibility?

Done. Hope nothing will break.

It's non obvious for now whether it's possible to use callback API with the specific agent. It should be documented at least. We can also make such APIs either SFINAE-friendly or add static_asserts in generated APIs with detailed reason (something like "Your agent is incompatible with callback-based API").

I think you better do this by yourself. :)

Smertig commented 1 year ago

Merged. Thanks for your contribution! 🎉