Skip to content

Conversation

@dgw
Copy link
Member

@dgw dgw commented Dec 6, 2023

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

  • I have read CONTRIBUTING.md
  • I can and do license this contribution under the EFLv2
  • No issues are reported by make qa (runs make lint and make test)
  • I have tested the functionality of the things this change touches

@dgw dgw added Tweak Breaking Change Stuff that probably should be mentioned in a migration guide labels Dec 6, 2023
@dgw dgw added this to the 8.0.0 milestone Dec 6, 2023
@dgw dgw requested a review from a team December 6, 2023 03:24
Copy link
Contributor

@SnoopJ SnoopJ left a 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.

Copy link
Contributor

@Exirel Exirel left a 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.

@dgw dgw force-pushed the implied-chanmsg-bot_priv branch from 3872427 to ff98f78 Compare December 9, 2023 03:52
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.
@dgw
Copy link
Member Author

dgw commented Dec 9, 2023

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. 🙂

Copy link
Contributor

@Exirel Exirel left a 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

@dgw dgw merged commit 6a25d17 into master Dec 9, 2023
@dgw dgw deleted the implied-chanmsg-bot_priv branch December 9, 2023 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Breaking Change Stuff that probably should be mentioned in a migration guide Tweak

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants