-
-
Notifications
You must be signed in to change notification settings - Fork 86
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
Add Starboard support #881
base: develop
Are you sure you want to change the base?
Conversation
application/config.json.template
Outdated
@@ -95,4 +95,8 @@ | |||
"rateLimitWindowSeconds": 10, | |||
"rateLimitRequestsInWindow": 3 | |||
} | |||
"starboard": { | |||
"emojiNames" : ["star"], | |||
"channelName": "starboard" |
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.
channelNamePattern
please (and in the code Pattern.compile(...)
then, matching the channel)
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.
In that case, I would only have one pattern and not an array right? The design you are referring to I presume is the one where if the user wants multiple emojis they would need to seperate it by a pipe?
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.
the emoij names can stay non-pattern like that. but the channel name should be a pattern, so that we have some flexibility in naming it.
* Gets the config for the Starboard Contains the List of emoji names recognized by the | ||
* starboard as well as the name of the channel with the starboard. |
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.
these details are outdated extremely quick, cause people editing the config wont notice that its duplicated here. get rid of that. instead, describe what a starboard is.
import java.util.Objects; | ||
|
||
@JsonRootName("starboard") | ||
public final class StarboardConfig { |
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.
missing javadoc
private final String channelName; | ||
|
||
@JsonCreator(mode = JsonCreator.Mode.PROPERTIES) | ||
public StarboardConfig( |
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.
missing javadoc
* Gets the name of the channel with the starboard Deactivate by using a non-existent channel | ||
* name | ||
* | ||
* @return the name of the channel with the starboard | ||
*/ | ||
public String getChannelName() { | ||
return channelName; | ||
} |
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.
(pattern, see earlier)
missing dot in the first and second sentence.
if (database.read(context -> context.selectFrom(STARBOARD_MESSAGES) | ||
.fetch("message_id") | ||
.contains(event.getMessageIdLong()))) { |
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.
too complex for a if
, put it into a helper method
.queue(); | ||
} | ||
|
||
private boolean ignoreMessage(String emojiName, Guild guild, GuildChannel channel) { |
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.
bad method name. its missing a shouldXXX
, otherwise it sounds as if this method, when called, will ignore the message
User author = message.getAuthor(); | ||
return new EmbedBuilder().setAuthor(author.getName(), null, author.getAvatarUrl()) | ||
.setDescription(message.getContentDisplay()) | ||
.build(); // TODO make footer with link and reacted emojis |
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.
TODO (dont forget)
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.
Sonar and CodeQL won't let me forget it. :)
CREATE TABLE starboard_messages | ||
(message_id BIGINT NOT NULL PRIMARY KEY | ||
) |
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.
formatting looks a bit weird with the (
if (database.read(context -> context.selectFrom(STARBOARD_MESSAGES) | ||
.fetch("message_id") | ||
.contains(event.getMessageIdLong()))) { | ||
database.write(context -> context.newRecord(STARBOARD_MESSAGES) | ||
.setMessageId(event.getMessageIdLong())); | ||
} |
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.
whats the point of the database entry? its not used anywhere right now. id guess u want to check whether it exists to not duplicate-post? but that check seems to be missing right now.
also, u will have to additionally put an in-memory cache as layer above the DB to prevent checking the DB a million times in 5 seconds when someone posts an announcement. u can look in some of the other classes for that, search for Caffeine
(cache).
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.
The database is being used for the check, that is what the if is for. However the current approach seems to be insanely slow as I fetch the entire list of entries.
I'll look into 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 think your missing my point on the question. what is the point of the database entry? it does not control any logic right now.
you have:
if (database doesnt contain it) {
put it into database
}
but thats it. what point does it serve, sitting in the db? theres nothing in ur code that would control anything based on it.
u likely intended to stop transfering the message if its contained in the db, i.e.
if (database does not contain it) {
copy message to starboard
}
but u dont have that. you always copy the message to the starboard, regardless of database
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.
Oh oops. Ur right mb 😅
5267e3f
to
9e129af
Compare
Uh oh. |
Issue seems to be resolved after fixing merge conflicts. Weird but I’ll take it. |
private static MessageEmbed formEmbed(Message message) { | ||
User author = message.getAuthor(); | ||
return new EmbedBuilder().setAuthor(author.getName(), null, author.getAvatarUrl()) | ||
.setDescription(message.getContentDisplay()) | ||
.appendDescription(" [Link](%s)".formatted(message.getJumpUrl())) | ||
.build(); | ||
} |
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.
can u also check for attachments and add that in the embed
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 set the image of the embed as the first image file in the attachments, if any.
SonarCloud Quality Gate failed.
|
aef2361
to
9a43a3a
Compare
|
39d1e71
to
2127d4f
Compare
|
This feature adds a Starboard.
Certain emojis which when added as a reaction to a message in a channel which everyone has permissions to, will be sent as an embed in the starboard channel.
Both the emojis and the channel are customizable through
config.json