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

Defer voteythumbs request (also prettified latex, and deferred xkcd) #126

Merged
merged 5 commits into from
Jun 25, 2023

Conversation

emadkhan713
Copy link
Contributor

@emadkhan713 emadkhan713 commented Jun 24, 2023

This PR should fix #101 somewhat.

However, some older commands should also be ported to the application-commands framework (advent (#68) and holidays come to mind), and subsequently deferred.

Also, this is my first OSS contribution, so I apologize if I've messed up in formatting / protocol for the PR.

@andrewj-brown
Copy link
Member

andrewj-brown commented Jun 24, 2023

holidays is fairly new; it looks like it doesn't have any app commands but that's because it actually has zero commands. It's just a cronjob that runs every day at 9am, generating the "Today is <thing> day!" message, so there's not really any need to "defer" a response per #101.

advent you're absolutely right about, it's a horrific monolith of a cog (and I'm saying this as the guy who wrote the starboard!) that probably should be completely stripped out and re-written. Especially considering we run giveaways, it's rather annoying that we don't have any way to easily link AoC participants to discord identities, nor opt-outs for the (many) alumni that have no desire to win.

@andrewj-brown
Copy link
Member

andrewj-brown commented Jun 24, 2023

As for this PR; lgtm and tested. If you could run black over the repo for code style (poetry run black uqcsbot) then I'd be happy to merge, although I'll leave it up just for a day or two so others can chime in if they wish.

@emadkhan713
Copy link
Contributor Author

ah right, I should've read a bit more into holidays then, soz.

and yeah, maybe I'll leave advent for later, might take a bit of a while to wrap my head around.

quick q: were xkcd or latex passing beforehand? because I didn't touch the parts that failed checks

@andrewj-brown
Copy link
Member

On my machine (and on github) black wants to reformat latex and xkcd. It's just indentation changes; not an actual problem with your code logic, just formatting.

Copy link
Member

@49Indium 49Indium left a comment

Choose a reason for hiding this comment

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

LGTM (once black passes)

@emadkhan713 emadkhan713 requested a review from 49Indium June 24, 2023 12:10
Copy link
Member

@49Indium 49Indium left a comment

Choose a reason for hiding this comment

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

Looks great. I'll give it some time for others to view, but can merge in soon.
I was too hasty. Black is still having some issues. I'm going to have a look at what it is.

@49Indium
Copy link
Member

Was very minor adjustments, which I think are due to our settings for black (a single line for xkcd.py and only a couple for latex.py). Running poetry run black uqcsbot was all I did.

@emadkhan713 emadkhan713 reopened this Jun 25, 2023
@emadkhan713
Copy link
Contributor Author

whoops! still learning the ropes, haha

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.

lgtm and merging

@andrewj-brown andrewj-brown merged commit 370a113 into UQComputingSociety:main Jun 25, 2023
@emadkhan713 emadkhan713 deleted the voteythumbs-defer branch June 25, 2023 14:40
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.

voteythumbs timeout (& general timeout) issues
3 participants