Idea: call api methods differently

[EdJoPaTo]

The current approach of calling API methods is somewhat repetitive.

bot.send_message(&SendMessageParams::…). From the method alone its already clear which params is required. Writing code with that requires first the method, then automated import of the params object and has some not that nice multi depth idents with the builder then.

bot.send_message(
    &SendMessageParams::builder()
        .chat_id(chat_id)
        .text("Hello world!")
        .build(),
)
.expect("Should work");    

---

One initial idea was to have a bot.send_message() -> SendMessageParamsBuilder with some sort of .call() at the end which still remembers its underlying bot. That would require all the params objects to keep a reference to the Bot. But would result in a cleaner method call:

bot.send_message()
    .chat_id(chat_id)
    .text("Hello world!")
    .build()
    .call()
    .expect("Should work");

This would either require some Trait for all the Params structs to have the .call() method or some new bon feature (@Veetaha 👀) to allow an own function to be called on build. This would also be somewhat messy with sync / async, therefore impractical.

---

With the idea that every Params struct would need an impl which allows for calling, how about something like this:

impl SendMessageParams {
    fn into_params(self) -> (Params, Files) { todo!(); }

    // these call methods are relatively easy to automate with macros as only into_params differs per method:
    pub fn call_sync(self, bot: &Bot) -> MethodResult<Message> {
        let (params, files) = self.into_params();
        bot.request("sendMessage", params, files)
    }
    // same for call_async
}

SendMessageParams::builder()
    .chat_id(chat_id)
    .text("Hello world!")
    .build()
    .call_sync(&bot)
    .expect("Should work");

I thought about having a generic trait, but the different method return types are not well suited for that.

Combining .build().call_sync(&bot) into a single method might already be possible with the IsComplete trait? (@Veetaha 👀)

// maybe like this?
// could be generated at the same time with the call_(a)sync methods
impl<T: IsComplete> SendMessageParamsBuilder<T> {
    pub fn call_sync(self, bot: &Bot) -> ReturnType { self.build().call_sync(bot) }
}

When this would be the preferred approach of calling API methods a name / module change might also be neat:

-frankenstein::api_params::SendMessageParams::builder()….build().call(&bot)
+frankenstein::method::SendMessage::builder()….build().call(&bot)

Having these in two modules should allow for both at the same time: api_params and methods. That way api_params can be deprecated and removed at a later date. Given that sending bytes from references would require breaking changes for all the params anyway, a clean cut might be favorable. But even with that, the api_params might be able to keep the current syntax while having additional clones internally. Having both would result in longer compile times tho.

[Veetaha]

  bot.send_message()
      .chat_id(chat_id)
      .text("Hello world!")
      .build()
      .call()
      .expect("Should work");

This would require using the method builder syntax like this:

#[bon::bon]
impl Bot {
    #[builder(on(String, into))]
    fn send_message(&self, chat_id: String, text: String) -> T { /**/ }
}

The problem here is that there isn't a solution implemented for shared configuration for methods with macro_rules_attribute, but it may be possible if bon adds an ability to specify on(...) clauses and other stuff at the level of the impl-global #[bon] macro (https://github.com/elastio/bon/issues/144).

Also, the problem of this approach is that the user no longer has a simple parameters struct that they could pass around in code that easily, because now the builder returned by send_message stores a &Bot reference internally. Also, for sync/async variations you'd need to define two functions (one sync and one async) with all the parameters repeated in each of them.

---

The easiest solution is as you mentioned with adding custom methods to the params struct builders with IsComplete bound on the builder's state because in this case you have just one params struct definition for both sync and async code.

Maybe you could even implement it with a more generic Into<Params> trait bound, but in this case you'd need to make some traits Call/AsyncCall for example (this would be possible to do with this feature implemented in bon: https://github.com/elastio/bon/issues/247):

impl<T: Into<Params>> Call for T {
    fn call(self, bot: &Bot) {/**/}
}

impl<T: Into<Params>> AsyncCall for T {
    async fn async_call(self, bot: &Bot) {/**/}
}

---

If you'd like to adopt the syntax of bot.method().arg().arg().call() without repeating the method args for sync/async variants, you could define the params structs with an internal #[builder(start_fn)] bot: &'a Bot field, and then define methods on the Bot that just delegate to the builder like this:

#[derive(bon::Builder)]
#[builder(finish_fn(vis = ""))]
struct Params<'a> {
    bot: &'a Bot,
    arg1: String,
    // ...
}

impl Bot {
    fn method(&self) -> ParamsBuilder<'_> {
        Params::builder(self)
    }
}

// Also implement the finishing methods for `ParamsBuilder`
impl<'a, S: params_builder::IsComplete> ParamsBuilder<'a, S> {
    fn call(self) -> T {
        let params = self.build();
        let bot = params.bot;
    }

    async fn async_call(self) -> T { /**/ }
}

[EdJoPaTo]

I guess one of the major pitfalls for this crate is the binding to the trait. All the methods are implemented on traits currently so that further complicates its usage with bon.

Also, both traits are very similar with a bunch of duplicated code and moving that to structs which are the same as long as possible for sync/async might actually be a good thing.

---

Maybe you could even implement it with a more generic Into<Params> trait bound, but in this case you'd need to make some traits Call/AsyncCall for example

These methods need to know their own method name so it's not as simple.

But something like this works surprisingly well:

type Files = Vec<InputFile>;
type Params = Option<serde_json::Map<String, Value>>;

trait Method {
    type Response: serde::de::DeserializeOwned;
    const NAME: &'static str;
    fn into_params(self) -> (Params, Files);

    fn call_sync<Bot: TelegramApi>(
        self,
        bot: &Bot,
    ) -> Result<MethodResponse<Self::Response>, Bot::Error>
    where
        Self: Sized,
    {
        // TODO: fully handle files
        let (params, _files) = self.into_params();

        bot.request(Self::NAME, params)
    }
}

impl Method for &SendMessageParams {
    type Response = Message;

    const NAME: &'static str = "sendMessage";

    fn into_params(self) -> (Params, Files) {
        let value = serde_json::to_value(self).unwrap();
        let map = match value {
            Value::Object(map) => map,
            _ => unreachable!("Method is implemented on struct and should always be object like"),
        };
        (Some(map), Vec::new())
    }
}

struct GetMe;
impl Method for &GetMe {
    type Response = User;

    const NAME: &'static str = "getMe";

    fn into_params(self) -> (Params, Files) {
        (None, Vec::new())
    }
}

fn main() {
    let chat_id = std::env::var("TARGET_CHAT")
        .unwrap()
        .parse::<i64>()
        .unwrap();
    let bot_token = std::env::var("BOT_TOKEN").unwrap();
    let bot = Bot::new(&bot_token);

    _ = dbg!(bot.get_me());
    _ = dbg!(GetMe.call_sync(&bot));

    let result = bot.send_message(
        &SendMessageParams::builder()
            .chat_id(chat_id)
            .text("Hello world!")
            .build(),
    );
    _ = dbg!(result);

    let result = SendMessageParams::builder()
        .chat_id(chat_id)
        .text("Hello world!")
        .build()
        .call_sync(&bot);
    _ = dbg!(result);
}

Also, notice the impl Method for & which allows having methods that don't need ownership while others need it (the ones with files probably).

Not sure how to do the .build().call_sync(&bot) shortcut in that example without introducing another trait. But that can probably wait for https://github.com/elastio/bon/issues/247

[Veetaha]

But that can probably wait for https://github.com/elastio/bon/issues/247

Note that it will generate additional several hundreds of From implementations, which is going to harm the compile times. It would be better to avoid that. The least compile time overhead would be to just allow call_* methods on the builder directly:

impl<S: send_message_params_builder::IsComplete> Method for SendMessageParamsBuilder<S> {
    type Response = Message;

    const NAME: &'static str = "sendMessage";

    fn into_params(self) -> (Params, Files) {
        let params = self.build();
        // ...
    }
}

[EdJoPaTo]

call_async (async fn on public trait) results in an unhappy Rust compiler:

https://doc.rust-lang.org/rustc/lints/listing/warn-by-default.html#async-fn-in-trait

[Veetaha]

Ah, right async fn in traits isn't recommended for public traits. Yeah, that's a bummer. This lint can be ignored, or you could use boxing with async-trait crate, but I think in this case it's not really needed