teloxide / teloxide

🤖 An elegant Telegram bots framework for Rust
https://docs.rs/teloxide/
MIT License
3.04k stars 227 forks source link

Feature Request: Add some syntactic sugar #1143

Open LasterAlex opened 3 weeks ago

LasterAlex commented 3 weeks ago

I think i came up with at least 5 instances where i would really like some additions inside of teloxide, not outside of it

1. JsonRequest.reply_to(msg) (or msg.id, make a trait like Into<MessageId> and implement for Message and MessageId)

Now: (replying and answering is different things!)

bot.send_dice(msg.chat.id).reply_parameters(ReplyParameters::new(msg.id)).await?;

After:

bot.send_dice(msg.chat.id).reply_to(msg).await?; // or
bot.send_dice(msg.chat.id).reply_to(msg.id).await?;

2. JsonRequest.disable_link_preview(true)

Now:

let options = LinkPreviewOptions {
    is_disabled: true,
    url: None,
    prefer_small_media: false,
    prefer_large_media: false,
    show_above_text: false,
};
bot.send_message(msg.chat.id, "https://github.com/teloxide/teloxide").link_preview_options(options).await?;

After:

bot.send_message(msg.chat.id, "https://github.com/teloxide/teloxide").disable_link_preview(true).await?;

3. bot.forward(to_chat_id, msg)

Now:

bot.forward_message(to_chat_id, msg.chat.id, msg.id).await?; // and
bot.forward_messages(to_chat_id, msg.chat.id, [msg_id_1, msg_id_2, msg_id_3]).await?;

After:

bot.forward(to_chat_id, msg).await?; // and
bot.forward(to_chat_id, [msg1, msg2, msg3]).await?;

We can make the input not just a Message, but we can also accept vecs of messages with IntoIterator<Message> and impl it to Message, because now we have copy_messages endpoint, but for ease of understanding maybe it's better to make bot.forward_many or smth If messages are from different groups - just panic or batch up the requests into multiple separate ones, up for debate

4. bot.copy(to_chat_id, msg)

Now:

bot.copy_message(to_chat_id, msg.chat.id, msg.id).await?; // and 
bot.copy_messages(to_chat_id, msg.chat.id, [msg_id_1, msg_id_2, msg_id_3]).await?;

After:

bot.copy(to_chat_id, msg).await?; // and 
bot.copy(to_chat_id, [msg1, msg2, msg3]).await?; 

5. bot.delete(msg)

Now:

bot.delete_message(msg.chat.id, msg.id).await?; // and 
bot.delete_messages(msg.chat.id, [msg_id_1, msg_id_2, msg_id_3]).await?;

After:

bot.delete(msg).await?; // and 
bot.delete([msg1, msg2, msg3]).await?;

Pros

It will be a lot easier to read than what is needed now (especially the reply_to and disable_link_preview, it now pains me to look at .reply_parameters(ReplyParameters::new(msg.id)) when it can be .reply_to(msg))

Cons

Alternatives

Don't do anything and leave as is, it is not necessary, or make a separate crate like teloxide_ext, but i feel like these things have value only if they are baked-in and you don't need to do anything aside from importing a trait




Currently waiting for @Hirrolot to review the proposal, and if approved - I can try to implement it when i have some time

Hirrolot commented 2 weeks ago

Although I can be strict at times with respect to syntax sugar, these suggestions look pretty straightforward, so I'm accepting the feature.

JsonRequest.reply_to(msg) (or msg.id, make a trait like Into<MessageId> and implement for Message and MessageId)

Into<MessageId> would be more flexible. (By the way, we don't need to implement Into<MessageId> for MessageId; it's done automatically.)

If messages are from different groups - just panic or batch up the requests into multiple separate ones, up for debate

Forwarding the messages from respective groups is more intuitive.

@LasterAlex, are you up to implementing this?

LasterAlex commented 2 weeks ago

Yes, absolutely! Will be glad to give it a shot in the near future! I, myself, would really want reply_to and delete to be implemented by the next release

LasterAlex commented 2 weeks ago

Forwarding the messages from respective groups is more intuitive.

Oh, there's a little problem with this, we can't return Self::ForwardMessages if we do that, and this is a whole different problem, i just realized that while trying to implement it

I will just put a panic in right now, I REALLY doubt people will try to forward two messages at the same time from different chats with the same function, and even if they do, the panic message will be clear enough to tell them to split the calls into separate ones

Hirrolot commented 2 weeks ago

I will just put a panic in right now

I wouldn't really like syntax sugar to have panics...

Why is it exactly impossible to implement forwarding from different groups?

LasterAlex commented 2 weeks ago

Well, it's not impossible, but it would remove all setter traits Maybe not detrimental, but, in my opinion, it's better to disallow multiple messages from different groups than deny the use of setters

The only solutions that would work without panics are:

1) Return a Result (basically let the user ? the error and not see why it doesn't work) 2) Make bot.forward() return a custom struct, implement the needed setters and .await to that struct, and make the return type a HashMap<ChatId, Vec<MessageId>> instead of just Vec<MessageId> 3) Unexpected behavior (sending only some messages or returning the setters for only one chat id)

Any of these ideas sound good? Second one seems the closest to what we want to do without panics, but it is a lot of code (I'm not against it, just the sense of cleanliness will fade), and it will need to be updated by hand when TBA decides to add some new field If the second one is good, I'll convert my pr into a draft

Hirrolot commented 1 week ago
  1. This is pretty much like a panic because users won't be checking for errors anyways.
  2. This is too complicated.
  3. Unexpected behaviour :)

Can we just implement forwarding only one message at a time?

LasterAlex commented 1 week ago

Ok, makes sense

LasterAlex commented 1 week ago

Also, @Hirrolot, while checking the examples i found that bot.edit_message_text and others like it could benefit from more simple syntax, e.g.

bot.edit_message_text(msg.chat.id, msg.id, text).await?;
// To
bot.edit_text(msg, text).await?;

And as new non-multi methods aren't too different from others, how about I look at all the requester methods and try to implement the shorter version where both the message.chat.id and message.id are required? It will make more consistent sugar (as well as more sugar).