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

Add Boost.Beast agent with callback interface. #18

Merged
merged 4 commits into from
Nov 22, 2022

Conversation

UltraCoderRU
Copy link
Contributor

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. :)

To disambiguate calls to API methods without real arguments (like log_out()) those methods now don't have empty args param.
@Smertig
Copy link
Owner

Smertig commented Nov 20, 2022

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
Copy link
Contributor Author

  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
Copy link
Owner

Smertig commented Nov 20, 2022

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.

Renames:
  make_future -> make_async
  beast_future_monadic -> beast_async_monadic
  beast_future -> beast_async
@UltraCoderRU
Copy link
Contributor Author

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 Smertig merged commit 9fa4efb into Smertig:master Nov 22, 2022
@Smertig
Copy link
Owner

Smertig commented Nov 22, 2022

Merged. Thanks for your contribution! 🎉

@UltraCoderRU UltraCoderRU deleted the async_api branch November 22, 2022 20:08
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.

2 participants