Team 8 by biskette · Pull Request #4 · python-discord/code-jam-1

biskette · 2018-03-23T00:47:34Z

The stuff 'n' that.

Fix Pipfile dependencies

Exclude virtualenv from Flake8 linting. Again.

jb3 · 2018-03-24T02:29:27Z

bot/cogs/snakes.py

 def setup(bot):
    bot.add_cog(Snakes(bot))
    log.info("Cog loaded: Snakes")
+    bot.loop.create_task(Snakes(bot).cache_snakelist())


Instead of creating two instances of the same class, the __init__ method of the class should create the task to cache the data.

Runew0lf wrote it.

Ruinew0lf fixed it.

…e-jam-1 into bisk # Conflicts: # bot/cogs/snakes.py

[UPDATE] Ignored a silly rule from flake8.

…e-jam-1 into bisk

Doing comments.

lemonsaurus

This is a pretty solid PR all in all. The game is fun, and appears to work well. Your get_snek is one of the better ones in the whole jam. all in all a job well done.

lemonsaurus · 2018-03-25T10:30:17Z

bot/cogs/snakes.py

+EMPTY = u'\u200b'
+
+# Results
+TICK_EMOJI = "\u2705"  # Correct Peg, Correct Hole


Why Are You Capitalizing Every Word In This Comment

lemonsaurus · 2018-03-25T10:30:25Z

bot/cogs/snakes.py

+# Results
+TICK_EMOJI = "\u2705"  # Correct Peg, Correct Hole
+CROSS_EMOJI = "\u274C"  # Wrong
+BLANK_EMOJI = "\u26AA"  # Correct Peg Wrong Hole


missing comma, lol

lemonsaurus · 2018-03-25T10:38:16Z

bot/cogs/snakes.py

+        # This caches the very expensive snake list operation on load
+        self.setup = bot.loop.create_task(self.cache_snake_list())
+
+    async def cache_snake_list(self):


This may be a little overengineered considering you only ever call get_snake_list() from this method. It may be better to just merge those two methods into one unless you plan on using get_snake_list for anything else.

lemonsaurus · 2018-03-25T10:39:13Z

bot/cogs/snakes.py

        """
-        Go online and fetch information about a snake
+        self.snake_cache = await self.get_snake_list()
+        return


This line does nothing. A method that has no return will also return None at the end of the method, so there's no reason to do it explicitly.

lemonsaurus · 2018-03-25T10:58:22Z

bot/cogs/snakes.py

+        snake_list = sorted(list(set(snake_list)))
+        return snake_list
+
+    async def get_snek(self, name: str = None) -> Dict[str, Any]:


the docstring for this method is bad.

lemonsaurus · 2018-03-25T11:16:43Z

bot/cogs/snakes.py

+            return (
+                # Conditions for a successful pagination:
+                all((
+                    # Reaction is on this message


these should probably be inline, and it'd be nice if they all lined up.

lemonsaurus · 2018-03-25T11:18:41Z

bot/cogs/snakes.py

+            )
+
+        while True:
+            try:


this is a huge try, which is an anti-pattern. You should only try the code that can actually fail.

lemonsaurus · 2018-03-25T11:19:34Z

bot/cogs/snakes.py

+                            page_guess_list[antidote_tries] = " ".join(antidote_guess_list)
+                            log.info(f"Guess: {' '.join(antidote_guess_list)}")
+
+                            # Now Check Guess


Okay Then, Captain CapWords.

lemonsaurus · 2018-03-25T11:20:13Z

bot/cogs/snakes.py

+                            page_result_list[antidote_tries] = " ".join(guess_result)
+                            log.info(f"Guess Result: {' '.join(guess_result)}")
+
+                            # Rebuild the board


pls no more floating comments.

lemonsaurus · 2018-03-25T11:21:42Z

bot/cogs/snakes.py

+                            # Redisplay the board
+                            await board_id.edit(embed=antidote_embed)
+
+                            if win is True:


why are you using a while True with conditions and breaks at the end instead of just putting the conditions in the while?

gdude2002

A very nice, readable PR. Since you're using async_timeout, you'll need to pipenv install it - or you could remove it, and stick with aiohttp.Timeout(interval).

gdude2002 · 2018-03-25T11:20:11Z

bot/cogs/snakes.py


+import aiohttp
+
+import async_timeout


You could just use aiohttp.Timeout

gdude2002 · 2018-03-25T11:27:54Z

bot/cogs/snakes.py

+    @command(name="antidote")
+    async def build_board(self, ctx: Context):
+        """
+        Antidote - Can you create the antivenom before the patient dies!


lemonsaurus

Nice try though.

lemonsaurus · 2018-03-25T12:30:13Z

bot/cogs/snakes.py

+TICK_EMOJI = "\u2705"  # Correct peg, correct hole
 CROSS_EMOJI = "\u274C"  # Wrong
-BLANK_EMOJI = "\u26AA"  # Correct Peg Wrong Hole
+BLANK_EMOJI = "\u26AA"  # Correct peg, wrong hle


hle? seriously? 👍

lemonsaurus · 2018-03-25T12:31:12Z

bot/cogs/snakes.py

 ANTIDOTE_EMOJI = [FIRST_EMOJI, SECOND_EMOJI, THIRD_EMOJI, FOURTH_EMOJI, FIFTH_EMOJI]


+def to_lower(argument):


I don't see why this helper function is useful. it's also error prone. if you pass anything into it that isn't a string, it'll crash the program. Just use the str.lower() method instead of having this helper function at all.

lemonsaurus · 2018-03-25T12:31:40Z

bot/cogs/snakes.py


    @command()
-    async def get(self, ctx: Context, name: str = None):
+    async def get(self, ctx: Context, name: to_lower = None):


you can't typehint something to a function. to_lower is not a type, and as far as I can tell name is still a str

You can do that, discord.py will convert the string to lowercase automatically

lemonsaurus · 2018-03-25T12:32:03Z

bot/cogs/snakes.py

+
+        # Check to see if the bot can remove reactions
+        if not ctx.channel.permissions_for(ctx.guild.me).manage_messages:
+            await ctx.send("Unable to start game as I dont have manage_messages permissions")


lemonsaurus · 2018-03-25T12:34:43Z

bot/cogs/snakes.py

-                    # There were no restrictions
-                    no_restrictions
+                    reaction_.message.id == board_id.id,  # Reaction is on this message
+                    reaction_.emoji in ANTIDOTE_EMOJI,  # Reaction is one of the pagination emotes


best to line these comments up with each other, much better readability. PEP8 only says there should be at least 2 spaces, but does not say anything about not being allowed to use five if it helps your program become more readable. so this:

something = something # do a useless thing something = not something # oh okay why did we even do the previous thing then

is encouraged.

biskette and others added 29 commits March 23, 2018 00:39

Merge pull request #1 from discord-python/master

eb1a327

Fix Pipfile dependencies

Stone by stone we'll build our castle walls...

6f85d43

We'll build them on common ground, and how!

895fefa

Merge pull request #2 from discord-python/master

f56b6c8

Exclude virtualenv from Flake8 linting. Again.

We all live in the space age, coming down with road rage...

8716582

Merge remote-tracking branch 'origin/master'

e4c4190

Travis is a bellend.

aa0aca1

Square peg, round hole.

2694897

Used wikipedia library to get initial data for testing

640e889

Now uses our own code for getting wikipedia api data

dc6aff5

Created function to grab json instead of repeating code

d838411

Wtf am i doing

b8d7356

Merge remote-tracking branch 'origin/master'

996570d

Created function to grab json instead of repeating code

c66cdd8

Merge remote-tracking branch 'origin/master'

98e4d6e

writing bollocks

751603b

Merge remote-tracking branch 'origin/master'

012aff8

fucking git eh?

c640b7a

Split out everything, fixed things, added things!

f31430a

Merge remote-tracking branch 'origin/master'

d900240

linted code and added comments

642762a

Mostly finished snake list

84a773a

Merge remote-tracking branch 'origin/master'

4081ecf

Mostly finished snake list

da1c851

Trimmed to necessary for integration

f7da5b8

Added bisks snake lookup

d9095e7

Caches snakelist on startup

fb0423f

Added check to make sure we're selecting a snake

b33bf1f

Removed wtfisthis

7015863

jb3 reviewed Mar 24, 2018

View reviewed changes

biskette and others added 17 commits March 24, 2018 11:08

Merge branches 'bisk' and 'master' of https://github.com/biskette/cod…

95cf6f3

…e-jam-1 into bisk # Conflicts: # bot/cogs/snakes.py

Linted code and started on antidote game

0f3acb4

Linted code and started on antidote game

684e921

Merge pull request #3 from discord-python/master

3c7e1cd

[UPDATE] Ignored a silly rule from flake8.

Linted code and finished basic game

75fb50c

Linted code and finished basic game

bd44a3f

Working on nicer return for ambiguous searches

96551b3

Making some checks for in snake_cache but -1 pageid

631d2ce

Stuff about a snake being in list but no page

a402c17

Cleaned up and tested get_snek function

dac90dd

Merge branches 'bisk' and 'master' of https://github.com/biskette/cod…

34ec1f8

…e-jam-1 into bisk

Fixed the flakey flake8.

d307309

Doing comments.

Wrote a doc string for get_snek

c7912d0

Wrote a bunch of comments explaining the Snake Finder General

2b9752a

Commented code to make it more legible to bisk

9f8b102

Merge remote-tracking branch 'origin/master'

44e0f30

Finished Game and added Win / Lose Messages

9b770ea

lemonsaurus reviewed Mar 25, 2018

View reviewed changes

gdude2002 suggested changes Mar 25, 2018

View reviewed changes

runew0lf added 4 commits March 25, 2018 12:52

Fixed some issues that lemon highlighted

d3300d1

Linted code

644f51f

Linted code and finished basic game

285ec33

Fixed while loop to include win / tries

1744c07

lemonsaurus suggested changes Mar 25, 2018

View reviewed changes

biskette and others added 6 commits March 25, 2018 13:44

Wrote a bunch of comments explaining the Snake Finder General

128234c

Remove cache

af7b064

Re-added cache because i broke something

4b53d9b

Wrote a bunch of comments explaining the Snake Finder General

29ad19d

Tidying up doc strings and comments

e1c5b03

Changed win screen to look tidy + include solution

cb26052

		ANTIDOTE_EMOJI = [FIRST_EMOJI, SECOND_EMOJI, THIRD_EMOJI, FOURTH_EMOJI, FIFTH_EMOJI]


		def to_lower(argument):


		import aiohttp

		import async_timeout

Uh oh!

Conversation

biskette commented Mar 23, 2018

Uh oh!

jb3 Mar 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

lemonsaurus 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!

gdude2002 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!

lemonsaurus 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jb3 Mar 24, 2018 •

edited

Loading