Roxxers / roxbot

Roxbot: An inclusive modular multi-purpose Discord bot. Built with love (and discord.py)
MIT License
21 stars 5 forks source link

;xkcd #12

Closed tcmal closed 6 years ago

tcmal commented 6 years ago

Grabs the image & metadata of the given xkcd comic from either the number, title or latest.

;xkcd 666
;xkcd Silent Hammer
;xkcd latest

Also made utils' delete thing clear the embed field of the deleted message (has no effect if it's not already gone)

Roxxers commented 6 years ago

Okay so there's some stuff I like and I don't like.

Good stuff

Not so good stuff

Point 1

Your argument parsing isn't the best.

I think it would be better UX design to do

async def xkcd(self, ctx, *, arg):

The above means you don't have to do the join for the title. Also means that if you provide no arg it won't provide a bad argument error. You can now slot something in that happens if no argument is given. Like the latest or a random one.

Point 2

You probably don't need a new cog for this. Its a pretty simple command.

Basically just an api call with some argument parsing overhead. So you could just put it in the fun cog. Which most fun api calls go. (frog tips, number facts, etc.)

Each cog usually has a different function and commands that are alike. Trivia is dedicated to the whole game, voice has the while voice client, twitch (how ever broken it might be right now) still only deals with twitch notifications.

I can't check how the command looks atm and you didn't provide that in a screencap so I can't comment.

Conclusion Due to the above, I'm probably gunna need those changes before I merge. I would like the additon of that random one. Possibly when no argument is given. Also moving it into the fun cog would be better than its own cog.

Roxxers commented 6 years ago

I gave incorrect code on the argument suggestion. Just realised it will still give an error if nothing is provided cause I dropped one of these boys

... *, arg = None):
tcmal commented 6 years ago

Sorry, forgot to take a screenshot.

2018-05-29-211650_1366x768_scrot

It might be bigger/smaller than usual, I'm on a laptop and have discord zoomed in a bit.

Finishing up those changes now, thanks.

Roxxers commented 6 years ago

Okay. So I am going to merge this but make a few changes as it's smaller stuff that would be a lil harder for me to pester you with. Thanks.