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

Reworked advent of code. #183

Merged
merged 12 commits into from
Nov 30, 2023
Merged

Reworked advent of code. #183

merged 12 commits into from
Nov 30, 2023

Conversation

49Indium
Copy link
Member

Refreshed all of advent of code to work with the new slash commands. This includes:

  • Adding a database to record AOC accounts and discord users
  • Adding a database to record previous winners
  • Reworking the command to select winners
  • Reworking the leaderboard display system

I will go over some of this tomorrow (including a full text guide that will be part of the wiki), but thought I'd get it out as soon as possible so that people can have a look. Note that to test the bot, you will need to fill out the environment variable AOC_SESSION_ID (this explains how to get it).

For a rough description of the new commands:
/advent help - Display help menu
/advent leaderboard - Display a leaderboard. Many sorting options and different leaderboard styles
/advent register - Register an AOC id to the current discord username. Used for registrating for prizes
/advent register-force - Force a registration between an AOC id and a discord user. Used for moderation and admin reasons
/advent unregister - Unregister an AOC id to the current discord username.
/advent unregister-force - Force-remove a registration between an AOC id and a discord user. Used for moderation and admin reasons
/advent previous-winners - Show the previous winners from a year
/advent new-winner - Add a discord user as a winner (chosen directly or by random selection) for prizes
/advent remove-winner - Remove a winner for the database

Refreshed all of advent of code to work with the new slash commands.
This includes:
 - Adding a database to record AOC accounts and discord users
 - Adding a database to record previous winners
 - Reworking the command to select winners
 - Reworking the leaderboard display system
@andrewj-brown andrewj-brown added the enhancement update an existing command or cog for some new functionality label Nov 20, 2023
Copy link
Member

@andrewj-brown andrewj-brown left a comment

Choose a reason for hiding this comment

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

Additionally, I have structural comments - I'd hoist out most, if not all, of the non-cog logic to a utils/aoc_utils.py file, especially the leaderboard filtering stuff down the bottom. Member should probably stay where it is, though.

The stuff that's left, sort by first-use - generally, someone reading the code top-to-bottom should be able to read in-order and not have to jump around too much to remind themselves of stuff. (e.g. Starboard, where I've mostly placed the four big interactions at the very bottom, and built functions up to them step-by-step)

I'll do a more thorough review tonight after work.

tests/test_advent.py Outdated Show resolved Hide resolved
uqcsbot/advent.py Outdated Show resolved Hide resolved
uqcsbot/advent.py Outdated Show resolved Hide resolved
uqcsbot/advent.py Outdated Show resolved Hide resolved
uqcsbot/advent.py Outdated Show resolved Hide resolved
uqcsbot/advent.py Outdated Show resolved Hide resolved
uqcsbot/advent.py Outdated Show resolved Hide resolved
uqcsbot/advent.py Outdated Show resolved Hide resolved
uqcsbot/advent.py Outdated Show resolved Hide resolved
uqcsbot/advent.py Outdated Show resolved Hide resolved
@katrinafyi
Copy link
Member

katrinafyi commented Nov 21, 2023

Feel free to ignore (and this is not against the work) but I've privately held that there's no need for such complex and single-use machinery in uqcsbot. Aside from verification, the prize giving is quite simply solved by a spreadsheet and a random.choice(). Of course, there is the argument for transparency but the committee is trusted with so much more anyways 🤷

Copy link
Member

@andrewj-brown andrewj-brown left a comment

Choose a reason for hiding this comment

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

migrating "outdated" comments that are still relevant

uqcsbot/advent.py Outdated Show resolved Hide resolved
uqcsbot/advent.py Show resolved Hide resolved
@andrewj-brown
Copy link
Member

Feel free to ignore (and this is not against the work) but I've privately held that there's no need for such complex and single-use machinery in uqcsbot. Aside from verification, the prize giving is quite simply solved by a spreadsheet and a random.choice(). Of course, there is the argument for transparency but the committee is trusted with so much more anyways 🤷

I think there are enough complicated and/or fiddly parts that code could help with for it to be worth it. Specifically the registration/verification stuff - while it was no different to rolling a dice in previous years, expanding the registration system with this PR removes all the annoying "do a draw -> they don't want it -> do a draw". Also weighting by number of stars, which is a bit of annoying math to do with a spreadsheet anyway.

If verification happens in the future, it'd be even more useful, because we'd have a better system than just "send me a screenshot logged-in and promise you didn't inspect-element it".

@JamesDearlove
Copy link
Member

My review doesn't carry a ton of weight, shout out to @andrewj-brown going through it, but lgtm. Nice work!

My only comments are whether the register commands should be ephemeral or not. I'm not really swayed either way but it might create a little bit of bot spam sitting around.

JamesDearlove
JamesDearlove previously approved these changes Nov 27, 2023
Copy link
Member

@andrewj-brown andrewj-brown left a comment

Choose a reason for hiding this comment

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

One more change.

Would still be nice to get the ID stuff done, because re-migrating that back to a DB with no IDs will be a right pain, but I think time is going to be our enemy this year.

uqcsbot/advent.py Outdated Show resolved Hide resolved
Copy link
Member

@andrewj-brown andrewj-brown 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!

uqcsbot/advent.py Show resolved Hide resolved
uqcsbot/advent.py Show resolved Hide resolved
@andrewj-brown andrewj-brown merged commit fa9fc29 into main Nov 30, 2023
3 checks passed
@49Indium 49Indium mentioned this pull request Mar 6, 2024
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement update an existing command or cog for some new functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve Advent of Code commands
4 participants