Snake cog by lemonsaurus · Pull Request #78 · python-discord/bot

lemonsaurus · 2018-05-13T21:11:36Z

No description provided.

…g. (#74) Original PR: python-discord/code-jam-1#2 Completes task: https://app.clickup.com/754996/757069/t/2ww7u

… for snake questions.

…some code smell.

…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.

…trols.

…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.

jchristgit

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

jchristgit · 2018-05-17T13:39:33Z

bot/__init__.py


    # If the command looks like a python syntax command, try to parse it.
-    if current == "(" or current == "[":
+    if current and current == "(" or current == "[":


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?

yep, you're right.

jchristgit · 2018-05-17T13:41:38Z

bot/cogs/snakes.py

+    HOURGLASS_EMOJI,
+    CROSSBONES_EMOJI,
+    ALEMBIC_EMOJI,
+]


do we need to modify this? if not, what about using a tuple instead?

changed all of these to tuples.

jchristgit · 2018-05-17T13:45:34Z

bot/cogs/snakes.py

+}
+
+# Zzzen of pythhhon constant
+ZEN = """


import this ZEN = ''.join(this.d.get(char) or char for char in this.s)

?

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)

naw. it's a fun way to do it, but infinitely less readable.

jchristgit · 2018-05-17T13:46:34Z

bot/cogs/snakes.py

+    """
+
+    wiki_brief = re.compile(r'(.*?)(=+ (.*?) =+)', flags=re.DOTALL)
+    valid = ('gif', 'png', 'jpeg', 'jpg', 'webp')


valid is a bit very generic. maybe valid_img_extensions?

jchristgit · 2018-05-17T13:47:22Z

bot/cogs/snakes.py

+            draw.text([margin + 4, offset], line, font=CARD['font'])
+            offset += CARD['font'].getsize(line)[1]
+
+        del draw


why the manual dels? isn't this garbage collected anywaysssssss?

jchristgit · 2018-05-17T13:57:25Z

bot/converters.py

+        """
+        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.


I've seen two invocations of this that are both within cogs. Can't we pass ctx.bot.http_session?

converters isn't a cog, there's no access to bot. but I'm sure there is a solution for this. I'll inquire.

jchristgit · 2018-05-17T14:00:24Z

bot/decorators.py

+            lock = func.__locks.setdefault(ctx.author.id, Lock())
+            if lock.locked():
+                return
+            async with func.__locks.setdefault(ctx.author.id, Lock()):


func.__locks is a WeakValueDictionary, and I'm not aware of WeakValueDictionary.__aenter__ being a thing. Am I missing something?

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.

Also it might be nice to return something to the user saying the command is already running

jchristgit · 2018-05-17T14:02:09Z

bot/utils/__init__.py

+        # Love that duplicate code
+        for coro in pending:
+            coro.cancel()
+    except asyncio.TimeoutError:


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?

actually there are more lines that can cause it, so this is fine.

jchristgit · 2018-05-17T14:02:36Z

bot/utils/__init__.py

+    :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.


Since this appears to be a general utility, might be nice to document the BadArgument exception raised.

jchristgit · 2018-05-17T14:03:38Z

bot/utils/snakes/perlin.py

+"""
+import math
+import random
+# Licensed under ISC


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

jchristgit

👍

jb3

Comments have been dispatched 👌

jb3 · 2018-05-19T09:30:13Z

bot/cogs/snakes.py

+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


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 )

jb3 · 2018-05-19T09:38:33Z

bot/cogs/snakes.py

+        This game was created by Lord Bisk and Runew0lf.
+        """
+
+        def event_check(reaction_: Reaction, user_: Member):


Seems like this should probably be just a lambda or moved out of the function into the class.

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.

jb3 · 2018-05-19T09:38:51Z

bot/cogs/snakes.py

+        Modified by juan and lemon.
+        """
+
+        def beautiful_pastel(hue):


Same thing here as above, this should probably go in the class instead of the function.

Here, however, I do agree. This is a general utility, and should be a helper method.

jb3 · 2018-05-19T09:43:15Z

bot/cogs/snakes.py

+            """
+            return message.author == ctx.message.author
+
+        def get_random_long_message(messages, retries=10):


Could be moved to class

moving to helper method.

jb3 · 2018-05-19T09:44:53Z

bot/cogs/snakes.py

+        Modified by lemon.
+        """
+
+        def predicate(message):


The simplicity of this leads me to think it could be a lambda, maybe even just a lambda passed into the .filter() call

yeah okay, this one is simple enough to turn into a lambda.

jb3 · 2018-05-19T09:47:12Z

bot/cogs/snakes.py

+            if user.avatar is not None:
+                avatar = f"https://cdn.discordapp.com/avatars/{user.id}/{user.avatar}"
+            else:
+                avatar = (


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.

jb3 · 2018-05-19T09:49:19Z

bot/cogs/snakes.py

+                content=f"{youtube_base_url}{data[num]['id']['videoId']}"
+            )
+        else:
+            log.warning(f"CRITICAL ERROR: YouTube API returned {response}")


This should definetely return some sort of error to the user.

jb3 · 2018-05-19T09:51:02Z

bot/cogs/snakes.py

+        )
+    # endregion
+
+    @snake_card.error


This doesn't have a region, less of a requirement but could it go in a region such as "Error Handlers" or something similar.

a bit weird to have a region for a single method, but I'll do it anyway.

jb3 · 2018-05-19T09:52:38Z

bot/decorators.py

+            lock = func.__locks.setdefault(ctx.author.id, Lock())
+            if lock.locked():
+                return
+            async with func.__locks.setdefault(ctx.author.id, Lock()):


Also it might be nice to return something to the user saying the command is already running

jb3 · 2018-05-19T09:53:37Z

bot/utils/snakes/hatching.py

+        ------```'''
+
+stages = [h1, h2, h3, h4]
+snakes = {


Compared to the immense amount of snakes we have in other parts of the cogs, could we not find any more for this part?

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.

Sounds good to me

Leon Sandøy and others added 10 commits April 27, 2018 15:08

Added the random_snake_name feature, created by Iceman

2360f21

Added Antidote to the snake_cog.

c8c5cca

Add the snake quiz from Team 7 - Mushy and Cardium - to the snakes co…

a9293e9

…g. (#74) Original PR: python-discord/code-jam-1#2 Completes task: https://app.clickup.com/754996/757069/t/2ww7u

Cleaned up the snake quiz

514a17f

Cleaning up snake_quiz further and integrating it towards the new API…

2375290

… for snake questions.

Bugfixes for Antidote and Quiz

55eec4e

Implemented the zzzen of pythhhon, by prithaj and andrew.

01e04af

Added the snake facts feature by Andrew and Prithaj. Also cleaned up …

36f2de7

…some code smell.

Implemented the snake_videos feature. Also made a fix for a bug with …

5bcb305

…https sessions on local aiohttp

Merge branch 'master' into snake_cog

c6acf33

lemonsaurus changed the title ~~[WIP, do not merge] Snake cog~~ Snake cog May 13, 2018

lemonsaurus added the s: WIP Work In Progress label May 13, 2018

Leon Sandøy and others added 16 commits May 14, 2018 00:46

Implemented Momo and kel's snake perlin noise gen draw feature

c5cf21e

Implemented the hatch feature from Momo and Kels PR

5b3535f

Implemented the snakemefeature from the momo and kel PR, with big mod…

9c14d52

…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.

Started on Snakes and Ladders, but want to refactor it to use reactio…

4f4f45f

…ns. Fixed up the perlin noise gen to generate random snake attributes.

Movie command, initial version

bdda2a8

Snakes and Ladders implemented and rewritten to use reactions for con…

1a6c194

…trols.

Merge branch 'snake_cog' of github.com:discord-python/bot into snake_cog

0244dbd

made the snek draw feature even more fabulous.

9a7a2aa

SAL, get_snek, perlin

701547d

Fixing some minor problems with startup log spam. The bot will now on…

1aba2d9

…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.

Merging master for some bugfixes

68e8a69

Pointed all relevant functions at their respective snake API endpoint…

1b81818

…s. Tested. All the data is now in our database, and everything appears to work.

Added the guessing game by Ava and eivl

d8572e8

Trailing comma, baby

1a179ec

Added snake cards.

b261043

Added the snakes.about command, and cleaned up the cog. Still got a c…

2e9c3e9

…ouple of bugs with snake_card, but other than that this is done.

lemonsaurus removed the s: WIP Work In Progress label May 16, 2018

Merge branch 'master' into snake_cog

cba9e30

jchristgit requested changes May 17, 2018

View reviewed changes

Leon Sandøy added 3 commits May 18, 2018 19:20

Some fixes for the snake cards. Cards now use the converter on the sn…

d784ca0

…ake input, so it can disambiguate just like .get. Also made the special cases like bob ross available to both .get and .card

Some fixes to address Volcyy's review.

1f19b28

Merge branch 'snake_cog' of github.com:discord-python/bot into snake_cog

cb3d110

jchristgit previously approved these changes May 18, 2018

View reviewed changes

Addressing comments by gdude on the site PR

18663a7

lemonsaurus dismissed jchristgit’s stale review via 18663a7 May 18, 2018 18:17

lemonsaurus requested review from AkiraSama, SirThane and jb3 May 18, 2018 20:47

jb3 requested changes May 19, 2018

View reviewed changes

Changes requested by Joseph

9f5f0ce

jb3 approved these changes May 19, 2018

View reviewed changes

jb3 merged commit ebf3b9e into master May 19, 2018

Inveracity deleted the snake_cog branch May 20, 2018 08:40

Uh oh!

Conversation

lemonsaurus commented May 13, 2018

Uh oh!

jchristgit left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jchristgit left a comment

Choose a reason for hiding this comment

Uh oh!

jb3 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment