-
-
Notifications
You must be signed in to change notification settings - Fork 403
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
plugin: require_bot_privilege()
implies require_chanmsg()
#2580
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had some nits, but I don't fundamentally object to this change since the decorator is fundamentally about channels, so I say let's do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @SnoopJ's review regarding the assert message. I have a slight negative opinion of the debug line. Other than that, this looks good to me.
3872427
to
ff98f78
Compare
If the bot needs a channel privilege for a given callable, it probably wants to do something related to channel operations, and triggering such a callable in response to a private message makes no sense.
Each assertion now has the message, and I skipped the logger call given @Exirel's valid point about "span" (🇫🇷? 😁) in the logs. Ready to be merged over the weekend unless one of y'all swings by with a last-minute objection. 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
valid point about "span" (🇫🇷? 😁)
Nah, just a stupid typo from a 🧠 💨
Ready to be merged over the weekend unless one of y'all swings by with a last-minute objection. 🙂
GO
Description
If the bot needs a channel privilege for a given callable, it probably wants to do something related to channel operations, and triggering such a callable in response to a private message makes no sense.
Sorry about trying to add something into 8.0.0 this late… I've been writing the 8.0 migration guide tonight, and when I ran across #2405 I realized that #2399 should have contemplated doing this too. If we're gonna "breaking change" privilege-decorator stuff, we should do it all in one major version.
If others agree that this is a good idea we can just merge the patch that's already ready to go and get on with finishing the release. (That's why I didn't open an issue first, to shorten the discuss -> implement -> review cycle.)
Checklist
make qa
(runsmake lint
andmake test
)