-
-
Notifications
You must be signed in to change notification settings - Fork 653
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
Snake cog #78
Conversation
…g. (#74) Original PR: python-discord/code-jam-1#2 Completes task: https://app.clickup.com/754996/757069/t/2ww7u
… for snake questions.
…https sessions on local aiohttp
…ifications. It no longer uses markov, it was just too slow to get it to do something interesting. It can also be passed a message to snakify that instead.
…ns. Fixed up the perlin noise gen to generate random snake attributes.
…ly try to post to DEVLOG if it's not in debug mode. Also added the new snake API endpoints and prepping for database handling of all snek related datapoints.
…s. Tested. All the data is now in our database, and everything appears to work.
…ouple of bugs with snake_card, but other than that this is done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some thoughts, mostly on the non-snake stuff, since that was written during the code jam and already went through code review. Mainly some questions. Looks good otherwise
bot/__init__.py
Outdated
@@ -241,7 +243,7 @@ def parse_python(buffer_pos): | |||
self.index += pos | |||
|
|||
# If the command looks like a python syntax command, try to parse it. | |||
if current == "(" or current == "[": | |||
if current and current == "(" or current == "[": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this needed? if current
is a string, bool(current) = len(current) > 0
. If current
is None
, as above, then current != "("
and current != "["
, or am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, you're right.
bot/cogs/snakes.py
Outdated
HOURGLASS_EMOJI, | ||
CROSSBONES_EMOJI, | ||
ALEMBIC_EMOJI, | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need to modify this? if not, what about using a tuple instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed all of these to tuples.
} | ||
|
||
# Zzzen of pythhhon constant | ||
ZEN = """ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import this
ZEN = ''.join(this.d.get(char) or char for char in this.s)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually, i forgot that spams console
import contextlib
import os
with contextlib.redirect_stdout(os.devnull):
import this
ZEN = ''.join(this.d.get(char) or char for char in this.s)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
naw. it's a fun way to do it, but infinitely less readable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:(
bot/cogs/snakes.py
Outdated
""" | ||
|
||
wiki_brief = re.compile(r'(.*?)(=+ (.*?) =+)', flags=re.DOTALL) | ||
valid = ('gif', 'png', 'jpeg', 'jpg', 'webp') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
valid
is a bit very generic. maybe valid_img_extensions
?
bot/cogs/snakes.py
Outdated
draw.text([margin + 4, offset], line, font=CARD['font']) | ||
offset += CARD['font'].getsize(line)[1] | ||
|
||
del draw |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why the manual del
s? isn't this garbage collected anywaysssssss?
""" | ||
This is stupid. We should find a way to | ||
somehow get the global session into a | ||
global context, so I can get it from here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've seen two invocations of this that are both within cogs. Can't we pass ctx.bot.http_session
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
converters isn't a cog, there's no access to bot
. but I'm sure there is a solution for this. I'll inquire.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
bot/decorators.py
Outdated
lock = func.__locks.setdefault(ctx.author.id, Lock()) | ||
if lock.locked(): | ||
return | ||
async with func.__locks.setdefault(ctx.author.id, Lock()): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
func.__locks
is a WeakValueDictionary
, and I'm not aware of WeakValueDictionary.__aenter__
being a thing. Am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this question is better directed at Ava, I have no idea what this code is doing and just blindly implemented it because Ava tends to know what she's doing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also it might be nice to return something to the user saying the command is already running
# Love that duplicate code | ||
for coro in pending: | ||
coro.cancel() | ||
except asyncio.TimeoutError: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't the await asyncio.wait
the only line that can raise an asyncio.TimeoutError
? Wouldn't it be better if we would put only that in the try
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually there are more lines that can cause it, so this is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, okay
:param per_page: Entries per embed page | ||
:param empty: Whether the paginator should have an extra line between items | ||
:param embed: The embed that the paginator will use. | ||
:return: Users choice for correct entry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this appears to be a general utility, might be nice to document the BadArgument
exception raised.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, done.
bot/utils/snakes/perlin.py
Outdated
""" | ||
import math | ||
import random | ||
# Licensed under ISC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why's this here twice?
…ake input, so it can disambiguate just like .get. Also made the special cases like bob ross available to both .get and .card
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments have been dispatched 👌
bot/cogs/snakes.py
Outdated
from discord.ext.commands import AutoShardedBot, BadArgument, Context, bot_has_permissions, command | ||
from PIL import Image, ImageDraw, ImageFont | ||
|
||
from bot.constants import ERROR_REPLIES, OMDB_API_KEY, SITE_API_KEY, SITE_API_URL, YOUTUBE_API_KEY |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be better in the format change you used in bot/cogs/events.py
from bot.constants import (
ERROR_REPLIES, OMDB_API_KEY, SITE_API_KEY,
SITE_API_URL, YOUTUBE_API_KEY
)
bot/cogs/snakes.py
Outdated
This game was created by Lord Bisk and Runew0lf. | ||
""" | ||
|
||
def event_check(reaction_: Reaction, user_: Member): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like this should probably be just a lambda or moved out of the function into the class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a lambda, the documentation would suck. no docstring or inline comments would make this much harder to read, so I don't think that would be an improvement.
Because this particular function has no use to anything else in the class, I don't think making it a class method is useful either. So, I'm keeping this the way it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌
bot/cogs/snakes.py
Outdated
Modified by juan and lemon. | ||
""" | ||
|
||
def beautiful_pastel(hue): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thing here as above, this should probably go in the class instead of the function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, however, I do agree. This is a general utility, and should be a helper method.
bot/cogs/snakes.py
Outdated
""" | ||
return message.author == ctx.message.author | ||
|
||
def get_random_long_message(messages, retries=10): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be moved to class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moving to helper method.
bot/cogs/snakes.py
Outdated
Modified by lemon. | ||
""" | ||
|
||
def predicate(message): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The simplicity of this leads me to think it could be a lambda, maybe even just a lambda passed into the .filter()
call
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah okay, this one is simple enough to turn into a lambda.
bot/cogs/snakes.py
Outdated
if user.avatar is not None: | ||
avatar = f"https://cdn.discordapp.com/avatars/{user.id}/{user.avatar}" | ||
else: | ||
avatar = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason we can't use the users default avatar (colored clyde image).
The way to get the link to these is through the ctx.author.default_avatar_url
attribute.
bot/cogs/snakes.py
Outdated
content=f"{youtube_base_url}{data[num]['id']['videoId']}" | ||
) | ||
else: | ||
log.warning(f"CRITICAL ERROR: YouTube API returned {response}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should definetely return some sort of error to the user.
bot/cogs/snakes.py
Outdated
) | ||
# endregion | ||
|
||
@snake_card.error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't have a region, less of a requirement but could it go in a region such as "Error Handlers" or something similar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a bit weird to have a region for a single method, but I'll do it anyway.
bot/decorators.py
Outdated
lock = func.__locks.setdefault(ctx.author.id, Lock()) | ||
if lock.locked(): | ||
return | ||
async with func.__locks.setdefault(ctx.author.id, Lock()): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also it might be nice to return something to the user saying the command is already running
------```''' | ||
|
||
stages = [h1, h2, h3, h4] | ||
snakes = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Compared to the immense amount of snakes we have in other parts of the cogs, could we not find any more for this part?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is perhaps something that can be PR'd in later. I do agree that it should have more, but I'm not gonna prioritize it now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me
No description provided.