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

feat: integrate the auto-thread plugin into the core lib #352

Open
EdJoPaTo opened this issue Jan 4, 2023 · 8 comments
Open

feat: integrate the auto-thread plugin into the core lib #352

EdJoPaTo opened this issue Jan 4, 2023 · 8 comments
Projects

Comments

@EdJoPaTo
Copy link
Member

EdJoPaTo commented Jan 4, 2023

ctx.reply() (and similar reply methods) do not reply to the current forum thread. They should as their use is to be a reply to whatever the current context is.

Not sure if this is a breaking change or not as it personally sounds like a bugfix to me.

Workaround until fixed:

await ctx.reply("something", {
  message_thread_id: 314,
});
@KnorpelSenf
Copy link
Member

This can already be done using https://github.com/grammyjs/auto-thread

@KnorpelSenf KnorpelSenf changed the title ctx.reply should reply to the current forum thread feat: integrate the auto-thread plugin into the core libc Jan 4, 2023
@KnorpelSenf KnorpelSenf changed the title feat: integrate the auto-thread plugin into the core libc feat: integrate the auto-thread plugin into the core lib Jan 4, 2023
@KnorpelSenf KnorpelSenf reopened this Jan 4, 2023
@EdJoPaTo
Copy link
Member Author

EdJoPaTo commented Jan 7, 2023

Personally I still think this is something the reply method is supposed to do on its own. The idea of reply is to reply to whatever the update was about. If that's in some thread then reply in some thread. If the reply method is not handling this its a bug to me as the idea of reply is violated.

Integrating some plugin sounds strange to me as its manipulating via a middleware and not natively correct behaviour which would be more efficient. Maybe that is what was meant to be done but the title of this issue should still be something like "reply doesnt work correctly with threads → fix that"

@KnorpelSenf KnorpelSenf added this to To do in Coding via automation Jan 24, 2023
@roziscoding
Copy link
Contributor

I agree that this should be the default behavior, but changing this now would lead to a change in behavior of existing bots and would require people who like the current behavior to refractor their code to use ctx.api.sendMessage, or ctx.sendMessage, if we add it.

Also, it'd require us to change the bahavior of all the replyWith... too, for consistency, which would require us to have ctx.send... methods for people who like the current behavior.

If we were to integrate this behavior into core, I'd do it being a config flag, and then we make it default to true on 2.0.

@KnorpelSenf
Copy link
Member

The current behaviour can be achieved via ctx.api methods. This is preferable over configuration.

Is it correct to leave out the parameter? It used not to be, but perhaps leaving it out will now send messages to the general topic?

I see two possible ways to proceed with this:

  1. Wait until 2.0.
  2. Label this a bug because it's unexpected behaviour, so we're allowed to fix it immediately without breaking semver.

@roziscoding
Copy link
Contributor

roziscoding commented Feb 1, 2023

The current behaviour can be achieved via ctx.api methods. This is preferable over configuration.

Yes, which means that, if we do point 2, we'll change the behavior of people's code without changing the major, and will require people to refactor their codebase to stay up to date, which should only really be expected on major releases

I understand that asking people to refactor is preferred over configuration, but that would break SemVer. If we're OK with breaking SemVer because we assume everybody expects reply to work this way, then fine. Otherwise, I'd suggest we go another route.

I'd also like to point out that using ctx.api methods will require the user to pass the chatId manually, and there won't be a method to send a message to the current chat without replying. If we do 2, we need to keep that in mind, and probably on the docs.

@KnorpelSenf
Copy link
Member

Yes, which means that, if we do point 2, we'll change the behavior of people's code without changing the major

I'm aware, the question remains if that isn't actually a good thing. If it's true that omitting the thread identifier refers to the general topic, then the current implementation is somewhat broken. If a user sends a message in a topic, the bot would always send an unrelated message in a different topic. Fixing ctx.reply would fix these bots.

@EdJoPaTo
Copy link
Member Author

EdJoPaTo commented Feb 1, 2023

Personally I still think this is a bug. Reply is meant to reply to exactly what the user did, so sending a message in a thread should be replied exactly there: in the thread.
The current behavior does not respect that and differs from that → bug.

I also cant see why people would want the current behaviour. reply is simply useless for threads currently and the dev needs to specify the thread id so the reply method works as expected. Fixing that in grammY will not require refactoring of bots as both the dev and grammy set the same thread id. The dev can remove that additional code but it will not break something. Or do I overlook something?

@roziscoding
Copy link
Contributor

I think I mixed things up.
@KnorpelSenf linked to this when I talked about joining the autoquote and autothread plugins, so I assumed we would integrate the autoquote plugin into core too.
I'm sorry. I completely agree that the autothread plugin should be incorporated into core.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: To do
Coding
To do
Development

No branches or pull requests

3 participants