-
Notifications
You must be signed in to change notification settings - Fork 20
Skill role refactor #148
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
base: main
Are you sure you want to change the base?
Skill role refactor #148
Conversation
|
eccce29 to
1c8e874
Compare
1c8e874 to
ff4c8ca
Compare
Prerequisite for #148 and #179, factored out for easier review: * introduce callbacks for other components to register role checks and be notified of role updates * recast role change detection logic into the language of sets * role update events only fire once new state has been committed, no need for debounce map anymore --------- Co-authored-by: Jeremy Rifkin <[email protected]>
a095f60 to
3b0d2ca
Compare
3b0d2ca to
8c32cdb
Compare
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 for taking the time to do this! Some initial comments:
| options: Discord.GuildMember | Discord.User | Discord.UserResolvable | Discord.FetchMemberOptions, | ||
| ) { | ||
| const member = await this.try_fetch_guild_member(options); | ||
| if (!member || (member.joinedAt && member.joinedAt.getDate() + 28 * DAY <= Date.now())) { |
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.
If a user leaves and rejoins this will be reset. I think ideally we'd look in the database for some information about how long they've been around.
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.
Well, the idea was that this would be a default implementation that provides a somewhat reasonable starting point out of the box. I'm not sure requiring a database for it is the way to go here?
| override async setup(commands: CommandSetBuilder) { | ||
| // const role_manager = unwrap(this.wheatley.components.get("RoleManager")) as RoleManager; | ||
| // role_manager.register_role_check(this.check_established.bind(this)); | ||
| this.wheatley.is_established_member = this.is_established_member.bind(this); |
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 sort of thing makes me pretty uneasy. If the check isn't suitable to just be implemented in wheatley, I think it'd be better for other components to just get handles to this component and use its logic.
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.
Well, the issue with that is that we have some components like anti-invite-links or anti-screenshots that I think we would like to just work out-of-the-box, independent of guild-specific components. Otherwise, we would probably have to move these into the TCCPP components?
|



Yet more untethering: make the bot core independent from skill role stuff.
SkillRolescomponent to house skill role logic and trackingWheatley.is_established_member()method to replace skill-role-based checks for screenshots and invite linksTheEstablishmentcomponent that overwritesWheatley.is_established_member()with TCCPP-specific logic.