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

Refector Python bot to Java #3

Merged
merged 22 commits into from
Nov 4, 2020
Merged

Refector Python bot to Java #3

merged 22 commits into from
Nov 4, 2020

Conversation

skyegallup
Copy link
Collaborator

Closes #2. This is a full re-implementation of the Python bot. It is not an identical copy; most notably, it features server-unique topic names and uses "topic" instead of "class". I will copy the other GitHub issues over from the old repository after this PR is closed.

@skyegallup
Copy link
Collaborator Author

I should be available for most of the weekend if anyone needs me to host the bot or provide the API key as part of their code review.

src/main/java/entities/Server.java Outdated Show resolved Hide resolved
src/main/java/entities/Server.java Outdated Show resolved Hide resolved
src/main/java/entities/Server.java Outdated Show resolved Hide resolved
src/main/java/entities/Server.java Outdated Show resolved Hide resolved
src/main/java/entities/Topic.java Outdated Show resolved Hide resolved
src/main/java/listeners/MainEventListener.java Outdated Show resolved Hide resolved
src/main/java/listeners/MainEventListener.java Outdated Show resolved Hide resolved
src/main/java/listeners/MainEventListener.java Outdated Show resolved Hide resolved
src/main/java/listeners/MainEventListener.java Outdated Show resolved Hide resolved
src/main/java/listeners/MainEventListener.java Outdated Show resolved Hide resolved
@Madrugaur
Copy link

@colbygallup

@skyegallup
Copy link
Collaborator Author

@Madrugaur

@Madrugaur Madrugaur self-requested a review October 26, 2020 04:19
Copy link

@Madrugaur Madrugaur left a comment

Choose a reason for hiding this comment

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

This all looks good to me, thanks for putting up with all my comments lmao

Copy link
Contributor

@peterkos peterkos left a comment

Choose a reason for hiding this comment

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

Great work!

A chunk of questions I have are not explicitly related to this code so feel free to write off as out-of-scope.

As mentioned in one of the comments, I haven't tested (or really considered) much of the actual functionality; here I'm more focused on the code quality itself. Later this week, or at next week's meeting, we'll take a closer look at what we intend to have and make a full feature list on Notion just to make sure the bot's on the path we want!

I'll also check in the Directors meeting on Tuesday to see if there's any functionality they'd like to see here.

src/main/java/entities/Server.java Show resolved Hide resolved
src/main/java/entities/Server.java Outdated Show resolved Hide resolved
src/main/java/entities/Topic.java Show resolved Hide resolved
src/main/java/entities/Topic.java Show resolved Hide resolved
src/main/java/launcher/Lehrry.java Outdated Show resolved Hide resolved
src/main/java/listeners/MainEventListener.java Outdated Show resolved Hide resolved
src/main/java/listeners/MainEventListener.java Outdated Show resolved Hide resolved
src/main/java/listeners/MainEventListener.java Outdated Show resolved Hide resolved
@skyegallup
Copy link
Collaborator Author

@peterkos Finished looking through your review. Fixed some, debated others, moved a few to separate issues. Let me know if these are all fine or if further changes are needed.

@skyegallup
Copy link
Collaborator Author

@peterkos Added most of the other suggestions. Just a couple more things to check, then we can merge this.

Copy link
Contributor

@peterkos peterkos left a comment

Choose a reason for hiding this comment

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

I'll approve; just needs this final change and is good!

src/main/java/listeners/MainEventListener.java Outdated Show resolved Hide resolved
@skyegallup skyegallup merged commit 07ed8c4 into develop Nov 4, 2020
@skyegallup skyegallup deleted the issue-2 branch November 5, 2020 19:30
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.

3 participants