-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
[Core] Multiple mod admin roles #2783
[Core] Multiple mod admin roles #2783
Conversation
- Adds Migration tool - Adds tool to migrate to allow multiple admin and mod roles - Supports Multiple mod and admin roles
I don't think we ever publicly documented the bot's db. I'm inclined to say that even if this may break someone, they should be using the exposed documented functions instead of this. I'm happy to review it later, and open to discussing delaying this for a breaking release if someone disagrees with that stance. |
A note for anyone going to test this: Since it modifies the bot's db including the types of things in it, I suggest testing with a brand new instance or a copy of a prior one. |
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 haven't had a chance to run this on a bot yet, however I looked over the strings a bit and have some changes to suggest.
-All doc strings: The spaces in between the text and the quotes should be removed for consistency with the rest of red
-All doc strings: Missing periods at the end.
-I remember a PR at some point added consistency to all of the del/delete/rem/remove commands by aliasing all of that, [p]set remove<mod/admin>role
might need to get the same treatment.
redbot/core/core_commands.py
Outdated
if role.id in roles: | ||
return await ctx.send(_("This is already an admin role.")) | ||
roles.append(role.id) | ||
await ctx.send(_("That role was added as an admin role for this guild.")) |
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.
This line and L758, "That role was added" doesn't sound very good, maybe a simple "Role added successfully."?
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 went with something different here, let me know if it sounds better to you.
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 need to do a bit more actual testing with this (not a full review), but it's what I've noticed so far.
Definitely liking most of what I'm seeing here so far.
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.
While I still don't love the "this" in "This role is already an <admin/mod> role.", the changes I requested have been addressed.
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.
Thanks @DiscordLiz for the quick follow up on issues discussed both here and in Discord.
🎉
Type
Description of the changes
This allows multiple mod and admin roles to be used.
I've updated everywhere which uses it directly.
If cog developers are expected to be allowed to interact with the bot's settings directly, this is also a breaking change.