Skip to content
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

13 starboard #96

Open
wants to merge 49 commits into
base: main
Choose a base branch
from
Open

13 starboard #96

wants to merge 49 commits into from

Conversation

Plyb
Copy link
Contributor

@Plyb Plyb commented Apr 15, 2023

Resolves #13

Copy link
Contributor Author

@Plyb Plyb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made a significant architecture change by adding the concept of reaction handlers. Instead of all reaction handling being done in messageReactionAdd.ts, that file now simply calls the execute message on separate ReactionHandlers. So duplicating reactions was pulled out into its own handler and starboard reaction handling is also in its own handler. In addition, there is a separate messageReactionRemove.ts. Both of these can consume reaction handlers, so we can reuse the starboard reaction handling for both.

README.md Show resolved Hide resolved
src/events/messageReactionAdd.ts Show resolved Hide resolved
src/main.ts Show resolved Hide resolved
@Plyb Plyb marked this pull request as ready for review April 15, 2023 18:28
@Plyb Plyb requested a review from a team April 15, 2023 18:28
ZYancey
ZYancey previously approved these changes Apr 18, 2023
Copy link
Contributor

@ZYancey ZYancey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

README.md Outdated Show resolved Hide resolved
prisma/schema.prisma Outdated Show resolved Hide resolved
prisma/schema.prisma Show resolved Hide resolved
src/reactionHandlers/add.ts Outdated Show resolved Hide resolved
src/reactionHandlers/remove.test.ts Outdated Show resolved Hide resolved
src/reactionHandlers/remove.ts Outdated Show resolved Hide resolved
src/reactionHandlers/add.test.ts Outdated Show resolved Hide resolved
src/@types/ReactionHandler.d.ts Outdated Show resolved Hide resolved
This reverts commit 44c980e.
@Plyb Plyb requested a review from a team July 8, 2023 20:50
JstnMcBrd
JstnMcBrd previously approved these changes Jul 11, 2023
AverageHelper
AverageHelper previously approved these changes Jul 16, 2023
@AverageHelper
Copy link
Contributor

K I'm gonna look at this today finally

@AverageHelper
Copy link
Contributor

Additional considerations:

  • Do we want reactboard messages to be deleted when their original message is deleted? Or are reactboards a deletion-resistant museum of sorts?
  • The bot occasionally joins reacts, but it ignores :star: reacts. It should probably ignore all other reactboard emojis, too. We can probs add this later, since right now we only really need the starboard working.

Copy link
Contributor

@AverageHelper AverageHelper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last comments and then I think it's ready:

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this file be called index.ts instead, following the pattern in src/events/?

Comment on lines +8 to +11
export const messageReactionRemove = onEvent('messageReactionRemove', {
once: false,
execute: buildExecute(allReactionHandlers),
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels weird for the 'messageReactionRemove' event to execute allReactionHandlers, instead of just a remove handler..

} else {
const customReact = await getCustomReact(guild, react);
if (customReact === undefined) {
throw new UserMessageError('React option must be a valid reaction');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For clarity:

Suggested change
throw new UserMessageError('React option must be a valid reaction');
throw new UserMessageError('React option must be a valid emoji');

I don't think you can send a "reaction" as a command argument per-se, but emojis are fair game :P

},
});

await replyPrivately('Reactboard created!');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally this would say something different depending on whether the board was created or updated or destroyed, but since we don't differentiate that rn, more generic language might be best:

Suggested change
await replyPrivately('Reactboard created!');
await replyPrivately('Reactboard set!');

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Starboard
4 participants