-
Notifications
You must be signed in to change notification settings - Fork 104
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
Comments
This can already be done using https://github.com/grammyjs/auto-thread |
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" |
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 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. |
The current behaviour can be achieved via 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:
|
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 I'd also like to point out that using |
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. |
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. 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? |
I think I mixed things up. |
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:
The text was updated successfully, but these errors were encountered: