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.

Sign up for GitHub

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

Jump to bottom

Members count display #1032

Closed
wants to merge 0 commits into from
Closed

Members count display #1032

wants to merge 0 commits into from

Conversation

devloves
Copy link
Contributor

issue #1028

@devloves devloves requested review from a team as code owners February 21, 2024 19:17
@CLAassistant
Copy link

CLAassistant commented Feb 21, 2024 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@tj-wazei tj-wazei self-requested a review February 21, 2024 19:37
devloves
devloves commented Feb 21, 2024
View reviewed changes
Copy link
Contributor Author

@devloves devloves left a comment

Choose a reason for hiding this comment

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

Did Changes mentioned by reviewers

Zabuzard
Zabuzard requested changes Feb 21, 2024
View reviewed changes
Copy link
Member

@Zabuzard Zabuzard left a comment

Choose a reason for hiding this comment

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

Welcome to the project and thank you very much for this PR. I got a few smaller things to tackle for you, shouldn't be that big of a deal. Feel free to ask👍

application/config.json.template Outdated Show resolved Hide resolved
application/config.json.template Outdated
@@ -110,5 +110,7 @@
"special": [
]
},
"memberCountCategoryName": "Info",
Copy link
Member

Choose a reason for hiding this comment

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

should be a pattern not just a string, change suffix to Pattern and code to use a Pattern.compile(...) and then pattern.matches(...) similiar to other entries in our config

Copy link
Member

Choose a reason for hiding this comment

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

not done

application/src/main/java/org/togetherjava/tjbot/config/Config.java Outdated
Comment on lines 416 to 415
* Gets the categories by this name to showcase the total member count of the server.
*
* @return the categories name types
Copy link
Member

Choose a reason for hiding this comment

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

i find this javadoc confusing wording-wise.

instead:

Gets the pattern matching the category that is used to display the total member count.

Copy link
Member

Choose a reason for hiding this comment

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

not changed

Copy link
Member

Choose a reason for hiding this comment

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

the return part still has confusing naming. perhaps the pattern matching the category

application/src/main/java/org/togetherjava/tjbot/features/Features.java Outdated Show resolved Hide resolved
application/src/main/java/org/togetherjava/tjbot/features/basic/MemberCounter.java Outdated Show resolved Hide resolved
application/src/main/java/org/togetherjava/tjbot/features/basic/MemberCounter.java Outdated Show resolved Hide resolved
application/src/main/java/org/togetherjava/tjbot/features/basic/MemberCounter.java Outdated Show resolved Hide resolved
application/src/main/java/org/togetherjava/tjbot/features/basic/MemberCounter.java Outdated

@Override
public Schedule createSchedule() {
return new Schedule(ScheduleMode.FIXED_RATE, 0, parseInt(updateMemberCountEveryHour),
Copy link
Member

Choose a reason for hiding this comment

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

just hardcore the number here and get rid of the var and config. u can go for one day

Copy link
Member

Choose a reason for hiding this comment

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

not done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

application/src/main/java/org/togetherjava/tjbot/features/basic/MemberCounter.java Outdated Show resolved Hide resolved
@devloves
Copy link
Contributor Author

Review Needed

@Zabuzard
Copy link
Member

Zabuzard commented Feb 22, 2024 •
edited
Loading

Could you please not resolve conversations urself? Just keep them open for the reviewer to check and resolve, thanks 👍
Cause majority of what I asked you to do isnt done yet, despite you marking it resolved. That makes me do double work and waste some time reviewing.

@devloves devloves force-pushed the develop branch 2 times, most recently from ea60d95 to 72f8b89 Compare February 22, 2024 08:59
@devloves
Copy link
Contributor Author

Could you please not resolve conversations urself? Just keep them open for the reviewer to check and resolve, thanks 👍 Cause majority of what I asked you to do isnt done yet, despite you marking it resolved. That makes me do double work and waste some time reviewing.

Hello yes sorry for that actually had some buggy commits

@devloves
Copy link
Contributor Author

My fault on that

Zabuzard
Zabuzard requested changes Feb 23, 2024
View reviewed changes
application/src/main/java/org/togetherjava/tjbot/features/basic/MemberCountDisplayRoutine.java Outdated
Comment on lines 25 to 27
if (baseName.contains(" Members")) {
baseName = Pattern.compile("(.+) - \\d+ Members").toString();
}
Copy link
Member

Choose a reason for hiding this comment

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

thats not really how the pattern thing works. ur supposed to compile that pattern beforehand, for example as private static final and then use it to match the given string, which returns a Matcher and that one has group(1) to extract the capturing-group (.+).

Copy link
Member

Choose a reason for hiding this comment

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

please test ur code. i would be surprised if this one works at all

application/src/main/java/org/togetherjava/tjbot/features/basic/MemberCountDisplayRoutine.java Outdated
import java.util.function.Predicate;
import java.util.regex.Pattern;

public class MemberCountDisplayRoutine implements Routine {
Copy link
Member

Choose a reason for hiding this comment

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

missing javadoc.

application/src/main/java/org/togetherjava/tjbot/features/basic/MemberCountDisplayRoutine.java Outdated
jda.getGuilds().forEach(guild -> {
guild.getCategories()
.stream()
.filter(cat -> wordsInCategory.test(cat.getName()))
Copy link
Member

Choose a reason for hiding this comment

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

write out cat to avoid confusion -> category

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

application/src/main/java/org/togetherjava/tjbot/features/basic/MemberCountDisplayRoutine.java Outdated
guild.getCategories()
.stream()
.filter(cat -> wordsInCategory.test(cat.getName()))
.forEach(this::updateCategoryName);
Copy link
Member

Choose a reason for hiding this comment

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

dont execute it forEach but only on the first/any occurence. use findAny()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@devloves
Copy link
Contributor Author

Been busy this week will work on the changes as i get time

@SquidXTV
Copy link
Member

Been busy this week will work on the changes as i get time

All good, take your time, it is low priority anyways.

@SquidXTV SquidXTV added enhancement New feature or request priority: low labels Feb 27, 2024
@christolis christolis closed this Mar 1, 2024
@devloves devloves mentioned this pull request Mar 1, 2024
Merged
@christolis
Copy link
Member

Closed PR as it was superseded by #1038!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SquidXTV SquidXTV SquidXTV left review comments

@surajkumar surajkumar surajkumar left review comments

@Zabuzard Zabuzard Zabuzard requested changes

@Taz03 Taz03 Taz03 requested changes

@tj-wazei tj-wazei Awaiting requested review from tj-wazei

Assignees
No one assigned
Labels
enhancement New feature or request priority: low
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@devloves @CLAassistant @Zabuzard @SquidXTV @christolis @Taz03 @surajkumar @tj-wazei