Add tophelpers command 🏆 #117

d3vel0per · 2021-09-14T13:18:47Z

Fixes #65

illuminator3

also maybe put the things into a different gradle module?

application/src/test/java/org/togetherjava/tjbot/commands/TopHelpersTest.java

application/src/test/java/org/togetherjava/tjbot/db/MessageMetadataListenerTest.java

build.gradle

application/src/main/java/org/togetherjava/tjbot/commands/TopHelpers.java

Tais993

+1 for using longs for ID's!

application/src/main/java/org/togetherjava/tjbot/commands/TopHelpers.java

application/src/main/java/org/togetherjava/tjbot/db/MessageMetadataListener.java

application/src/main/java/org/togetherjava/tjbot/util/PresentationUtils.java

application/src/main/resources/db/V2__table_message_metadata.sql

d3vel0per · 2021-09-18T20:48:15Z

Changed to draft. Should not be merged.
Waiting for a few other PR's to go in.

Zabuzard · 2021-09-25T20:37:26Z

@d3vel0per Any news on this?

d3vel0per · 2021-09-26T07:41:37Z

@d3vel0per Any news on this?

Yes, I am waiting for:
#119 - Separate thread pool for command system + AbstractCommand
#135 - Database migration and segregation of module

Since both will need merges at my end.

Zabuzard · 2021-10-14T15:41:21Z

Tell me if you need help with rebasing.

By the way, the database related things you have been waiting for with #135 found their way into develop already via different PRs. I am unblocking this.

d3vel0per · 2021-10-17T19:00:57Z

Tell me if you need help with rebasing.

By the way, the database related things you have been waiting for with #135 found their way into develop already via different PRs. I am unblocking this.

Should be good. I will pick it up again.

application/src/test/java/org/togetherjava/tjbot/listener/MessageMetadataListenerTest.java

java-coding-prodigy · 2021-10-20T14:13:02Z

We seem to be having some trouble with the formatting, I think we should wait till @I-Al-Istannen and @borgrel fix it.

application/src/main/java/org/togetherjava/tjbot/listener/MessageMetadataListener.java

Zabuzard · 2021-10-22T12:07:14Z

application/src/main/java/org/togetherjava/tjbot/listener/MessageMetadataListener.java

+/**
+ * Listener responsible for persistence of text message metadata
+ */


Please elaborate a bit here what and when it stores.

Zabuzard · 2021-10-22T12:08:46Z

application/src/main/java/org/togetherjava/tjbot/listener/package-info.java

@@ -0,0 +1,4 @@
+/**
+ * This package contains non-user interactive event listeners


Missing dot .

Maybe also add a link to JDAs addEventListener thing so that people understand what its about.

application/src/main/java/org/togetherjava/tjbot/util/JDAUtils.java

Zabuzard · 2021-10-22T12:10:39Z

application/src/main/java/org/togetherjava/tjbot/util/JDAUtils.java

+    private static final Pattern HELP_CHANNEL_PATTERN = Pattern.compile(".*\\Qhelp\\E.*");
+
+    private JDAUtils() {}
+
+    /**
+     * Checks if a provided channel is a help channel.
+     * 
+     * @param textChannel provided channel.
+     * @return true if the provided channel is a help channel, false otherwise.
+     */
+    public static boolean isAHelpChannel(TextChannel textChannel) {
+        return HELP_CHANNEL_PATTERN.matcher(textChannel.getName()).matches();
+    }


For now, I think it might be better to move those helpers back into where you actually use them, into TopHelperCommand or similar.

Zabuzard · 2021-10-22T12:37:10Z

application/src/main/java/org/togetherjava/tjbot/commands/TopHelpers.java

+    private static long getBoundedCountAsLong(OptionMapping count) {
+        return count == null ? MIN_COUNT : Math.min(count.getAsLong(), MAX_COUNT);
+    }


Instead of silently accepting inputs outside of the range, I would explicitly check the range and give the user an ephemeral message back, telling them to use the command with values inside the range and stop the command.

application/src/main/java/org/togetherjava/tjbot/commands/TopHelpers.java

Zabuzard · 2021-10-22T12:38:20Z

application/src/main/java/org/togetherjava/tjbot/commands/TopHelpers.java

+        });
+    }
+
+    private static long getBoundedCountAsLong(OptionMapping count) {


I ll need to cross check once again but I think it comes as null when not specified. It's one of the not required fields

Okay, then @Nullable. Unless your context requires it to always exist.

application/src/main/java/org/togetherjava/tjbot/commands/TopHelpers.java

illuminator3 · 2021-10-23T15:17:55Z

Please put everything that doesn't need to be in the application module in its own module.

Zabuzard · 2021-10-23T17:56:11Z

Please put everything that doesn't need to be in the application module in its own module.

U mean the whole command should be in its own module? We havent done that so far. Or what do you specifically mean in this PR?

Zabuzard · 2021-10-26T08:20:00Z

application/src/main/java/org/togetherjava/tjbot/commands/TopHelpers.java

 import org.jetbrains.annotations.Nullable;
 import org.jooq.Records;
 import org.togetherjava.tjbot.db.Database;
 import org.togetherjava.tjbot.util.PresentationUtils;

+import javax.annotation.ParametersAreNonnullByDefault;


We have not used javax.annotation yet. I would suggest to not use them

But why? Why would we prefer a vendor specific annotation and I see no point putting @NotNull/@nullable annotations across each and every method that we write.
If we prefer non-standard third party annotations, its still fine but does it have an equivalent?

The annotation we are using is already in our dependencies. If you want to use this other annotation, please make sure that the dependencies cover it.

That said, for the null-checks it would probably be best to stick to one and not mix the millions of @NotNull annotations with each other.

I dont have a strong opinion here. Maybe get other opinions (@I-Al-Istannen )

I'll have to side with Zabuzard.

Usage of @NotNull and @Nullable makes it clear what the arguments are.
When you look at the docs of a method, it actually shows the arguments can't be null or are nullable. (Because it displays the method signature)
If you use a class-wide annotation like this it's not shown in the docs either.

If we prefer non-standard third party annotations, its still fine but does it have an equivalent?

To my knowledge, javax annotations have been removed since Java 11, so this argument is obsolete.

Tais993 · 2021-10-26T14:37:00Z

application/src/main/java/org/togetherjava/tjbot/commands/TopHelpers.java

 import org.jetbrains.annotations.Nullable;
 import org.jooq.Records;
 import org.togetherjava.tjbot.db.Database;
 import org.togetherjava.tjbot.util.PresentationUtils;

+import javax.annotation.ParametersAreNonnullByDefault;


I'll have to side with Zabuzard.

Usage of @NotNull and @Nullable makes it clear what the arguments are.
When you look at the docs of a method, it actually shows the arguments can't be null or are nullable. (Because it displays the method signature)
If you use a class-wide annotation like this it's not shown in the docs either.

If we prefer non-standard third party annotations, its still fine but does it have an equivalent?

To my knowledge, javax annotations have been removed since Java 11, so this argument is obsolete.

Tais993 · 2021-11-03T09:07:23Z

application/src/main/java/org/togetherjava/tjbot/commands/TopHelpers.java

+
+public final class TopHelpers extends SlashCommandAdapter {
+
+    private static final String COUNT_OPTION = "count";


Zabuzard is also referring to the variable's name if I understand it correctly

Tais993 · 2021-11-03T09:08:10Z

application/src/main/java/org/togetherjava/tjbot/util/PresentationUtils.java

+public final class PresentationUtils {
+    private PresentationUtils() {}


He said "or"

So, why did you ignore his second statement?

Tais993 · 2021-11-03T09:09:54Z

application/src/main/java/org/togetherjava/tjbot/commands/TopHelpers.java

+    private static final long MIN_COUNT = 10L;
+    private static final long MAX_COUNT = 30L;
+
+    private record TopHelperRow(Integer serialId, Long userId, Long messageCount) {


I'd use the primitives instead

Zabuzard · 2021-11-07T12:04:56Z

@d3vel0per How is it going? Still planning on finishing this? Otherwise, should we maybe assign it to someone else to help out? (no hurry though)

CLAassistant · 2021-11-15T11:56:11Z

All committers have signed the CLA.

Zabuzard · 2021-11-16T12:41:52Z

Seems @d3vel0per is a bit busy right now (which is totally fair). Unassigned. Someone else can take over, if wanted 👍

marko-radosavljevic · 2021-11-26T15:16:29Z

Since origin branch of this PR is on a fork, people cannot work on it.

Moved to #282. ☺️

Thanks, @d3vel0per ❤️

illuminator3 requested changes Sep 14, 2021

View reviewed changes

illuminator3 assigned d3vel0per Sep 14, 2021

d3vel0per force-pushed the feature/tophelpers branch from b35f4e2 to d6dbd6e Compare September 14, 2021 13:25

illuminator3 added the enhancement New feature or request label Sep 14, 2021

d3vel0per requested review from Tais993, I-Al-Istannen, Zabuzard and marko-radosavljevic September 14, 2021 13:32

Zabuzard added the new command Add a new command or group of commands to the bot label Sep 14, 2021

Zabuzard added this to the Migrate existing commands milestone Sep 14, 2021

Zabuzard requested changes Sep 14, 2021

View reviewed changes

application/src/main/java/org/togetherjava/tjbot/commands/TopHelpers.java Outdated Show resolved Hide resolved

Tais993 reviewed Sep 14, 2021

View reviewed changes

d3vel0per marked this pull request as draft September 17, 2021 14:07

Zabuzard added blocked This issue is currently blocked by another issue (see comments) priority: normal labels Sep 27, 2021

marko-radosavljevic mentioned this pull request Oct 13, 2021

Tag system #135

Merged

Zabuzard removed the blocked This issue is currently blocked by another issue (see comments) label Oct 14, 2021

d3vel0per force-pushed the feature/tophelpers branch from d6dbd6e to afe1636 Compare October 20, 2021 08:34

d3vel0per marked this pull request as ready for review October 20, 2021 08:45

d3vel0per requested review from a team as code owners October 20, 2021 08:45

Zabuzard requested changes Oct 20, 2021

View reviewed changes

application/src/test/java/org/togetherjava/tjbot/listener/MessageMetadataListenerTest.java Outdated Show resolved Hide resolved

d3vel0per force-pushed the feature/tophelpers branch from afe1636 to 02dbe0a Compare October 22, 2021 05:34

d3vel0per requested a review from Zabuzard October 22, 2021 05:42

d3vel0per requested review from Tais993 and illuminator3 October 22, 2021 05:42

Zabuzard requested changes Oct 22, 2021

View reviewed changes

Zabuzard mentioned this pull request Oct 23, 2021

purge/clear command #17

Closed

d3vel0per added 2 commits October 26, 2021 11:46

TopHelpers with review and upstream changes

90c7a75

Accomodating review comments + upstream changes

40cddc0

d3vel0per force-pushed the feature/tophelpers branch from 02dbe0a to 40cddc0 Compare October 26, 2021 06:24

Updates based on review comments

20dffc7

Zabuzard reviewed Oct 26, 2021

View reviewed changes

Tais993 requested changes Nov 3, 2021

View reviewed changes

Zabuzard unassigned d3vel0per Nov 16, 2021

marko-radosavljevic mentioned this pull request Nov 26, 2021

Add top-helpers command #282

Merged

marko-radosavljevic closed this Nov 26, 2021

		@@ -0,0 +1,4 @@
		/**
		* This package contains non-user interactive event listeners


		public final class TopHelpers extends SlashCommandAdapter {

		private static final String COUNT_OPTION = "count";

		public final class PresentationUtils {
		private PresentationUtils() {}

Uh oh!

Add tophelpers command 🏆 #117

Add tophelpers command 🏆 #117

Uh oh!

Conversation

d3vel0per commented Sep 14, 2021

Uh oh!

illuminator3 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Tais993 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

d3vel0per commented Sep 18, 2021

Uh oh!

Zabuzard commented Sep 25, 2021

Uh oh!

d3vel0per commented Sep 26, 2021

Uh oh!

Zabuzard commented Oct 14, 2021

Uh oh!

d3vel0per commented Oct 17, 2021

Uh oh!

Uh oh!

java-coding-prodigy commented Oct 20, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

illuminator3 commented Oct 23, 2021

Uh oh!

Zabuzard commented Oct 23, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tais993 Oct 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tais993 Oct 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Tais993 Oct 26, 2021 •

edited

Loading

Tais993 Oct 26, 2021 •

edited

Loading

Zabuzard commented Nov 7, 2021 •

edited

Loading

CLAassistant commented Nov 15, 2021 •

edited

Loading