Skip to content

[BUG] Shortcuts lost their *args and **kwargs, resulting messagequeue isgroup= and other broke #2392

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

Closed
standin-fecha opened this issue Feb 18, 2021 · 7 comments

Comments

@standin-fecha
Copy link

There is no complicated stuff, someone just removed all *args and **kwargs from shortcuts such as reply_text in message, so old methods used with messagequeue wrapper and isgroup parameter stopped working because they simply can't pass this parameteres to strictly defined function. It's especially strange in case that messagequeue itself wasn't removed and from direct send_message methods works fine.

@Poolitzer
Copy link
Member

Hey, thank you for your issue report.

We introduced strongly typed shortcuts in our latest Version, 13.2. We also got rid of the kwargs and args parameters there, because they inconveniently hid typing errors. If you want to pass kwargs manually, we introduced api_kwargs as an argument where you can pass the kwargs as a dict.

Sadly, you said we broke messagequeue in the process. Can I ask you to provide a minimal working example (see here for what that is), so it is easier for me to do the debugging?

Btw, probably unrelated note, but we are going to give MQ a massive overhaul once we have proper async support, see #2139

@standin-fecha
Copy link
Author

Thanks a lot for the answer, a bit ugly construction api_kwargs={'isgroup': True} saved beautiful formal isgroup=True.

If I'm not wrong, it wasn't directly mentioned in changelog, so I missed it when the update came out and found strange error about it only now, while updating another Bot.

messagequeue itself working as usual, I'm talking only about shorcuts, which lost their direct option to pass isgroup without any error highlights or something until exact executing.

Yep, I knew about coming MQ refactoring, but it's open since 12 October and still in work, so feel a bit hopeless to finally get it. :) Working Defaults are the most wanted feature in it as for code beauty lover.

@Poolitzer
Copy link
Member

Thanks a lot for the answer

No problem :)

a bit ugly construction api_kwargs={'isgroup': True} saved beautiful formal isgroup=True.

Agreed.

If I'm not wrong, it wasn't directly mentioned in changelog,

It was one of the main features. (Explicit Signatures for Shortcuts (#2240). see release notes)

and found strange error about it

I guess we could've warned more about this, yeah. It didn't occur to me that MQ would use kwargs directly, never used it though.

which lost their direct option to pass isgroup without any error highlights or something until exact executing.

Hm, we might be able to get it back though. @Bibo-Joshi?

@standin-fecha
Copy link
Author

Ah, yeah, messed up with my English a bit about term Explicit.

As MQ rewrites (if I'm correct in py terms) send_message, for shortcuts it was working just as redirect (cite directly from file changes):
Before: def reply_text(self, *args: Any, **kwargs: Any) -> 'Message':
After: def reply_text(self, text: str, ...

So without isgroup in variables error is just like it is - "reply_text don't have isgroup".

@Bibo-Joshi
Copy link
Member

Allowing to pass arbitrary kwargs again in the shortcuts is not an option IMO as that would go directly against the reason why we dropped that in the first place. And in order to have api_kwargs={'is_group': True} work with MessageQueue, we'd have to rewrite the queuedmessage decorator to read that value form api_kwargs instead of directly from **kwargs (or to try both).

TBH I don't think it's worth it: v13.2 already breaks shortcuts for MQ and having the users switch to api_kwargs={'is_group': True} is probably not much more work than having them switch to calling the bot methods manually. Moreover, MQ already is an sparsely tested (that's why we didn't realize that v13.2 would break that …), unmainainted mess and I don't see the point of duct-taping the exhaust pipe into place on a car that's on its way to the scrapyard, so to speak.

I'd vote to just officially deprecate MQ, raising a warning at runtime and adding notes to the docs & wiki. Can be something along the lines

MQ in it's current form is deprecated and will be reinvented in a future release. See #2139 for a list of known bugs.

@Poolitzer
Copy link
Member

I'd vote to just officially deprecate MQ, raising a warning at runtime and adding notes to the docs & wiki.

I'm fine with it, as long as we stick to redoing it in V14. I think its an important part of our library

@Bibo-Joshi
Copy link
Member

All right, I'll prepare something. Added this issue in #2139 for reference. So I'll close in favor of #2319

@github-actions github-actions bot locked and limited conversation to collaborators Feb 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants