Skip to content
This repository was archived by the owner on Mar 14, 2021. It is now read-only.

Team 18 - #7

Open
katlyn wants to merge 48 commits into
python-discord:masterfrom
katlyn:master
Open

Team 18#7
katlyn wants to merge 48 commits into
python-discord:masterfrom
katlyn:master

Conversation

@katlyn

Copy link
Copy Markdown

Woot

Add a print statement so we can open our pull request
@gdude2002gdude2002 changed the title WIP Team 18Team 18Mar 23, 2018

@lemonsauruslemonsaurus left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

get_danger is both a fun idea, and particularly well written. rattle.py should match this level of quality. Docstrings, type hinting and some more comments would help a lot.

Comment threadtools/rattle.py Outdated
h = len(otherword) - len(word)
word += '~' * h
return word
'''

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

docstring before helper functions please.

Comment threadtools/rattle.py Outdated
@@ -0,0 +1,45 @@
# rattle - easy word similarity detection
def check_word(word, wordset, threshold=0.8):
def get_shortest(x, y):

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

These helper functions should also have docstrings.

Comment threadtools/rattle.py Outdated
length_diff_multiplier = 1.08
mp_count = 1
shorter = ''
if shortest == word:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This function desperately needs block comments. runs a bit hot.

Comment threadbot/cogs/snakes.py Outdated
from discord import Embed
from discord.ext.commands import AutoShardedBot, Context, command

from ..tools import rattle

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please no relative imports.

Comment threadbot/cogs/snakes.py Outdated
log = logging.getLogger(__name__)
db = load(open('bot/cogs/snek.pickledb', 'rb')) # are we going to move this db elsewhere?
SNAKE_NAMES = db.keys() # make a list of common names for snakes, used for random snake and autocorrect
DEBUG = True

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hardcoded debug mode?

Comment threadbot/cogs/snakes.py
"automatically fetched and may not be accurate.")
await ctx.send(embed=embed)

# Any additional commands can be placed here. Be creative, but keep it to a reasonable amount!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

With 12 hours to go, you have no extra functionality!

Because we need more functionality .

@Zwork101Zwork101 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Very few issues, and nothing major. You did a great job documenting the project making your code clear. Can't wait to see what you'll make next Jam!

Comment threadbot/cogs/events.py
parent = command.parent

if parent and command:
help_command = (self.bot.get_command("help"), parent.name, command.name)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Including the ( and ) is unnecessary, you can accomplish the same feat with simply

help_command=self.bot.get_command("help"), parent.name, command.name

Comment threadbot/cogs/events.py
f"Sorry, an unexpected error occurred. Please let us know!\n\n```{e}```"
)
raise e.original
log.error(f"COMMAND ERROR: '{e}'")

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This isn't logged if a non-expected CommandInvokeError happens. You could either wrap the ifs in a try - finally block, however just logging the error before you raise it would be fine.

Comment threadbot/cogs/events.py

log.debug(f"{before.display_name} roles changing from {before_role_names} to {after_role_names}")

if OWNER_ROLE in role_ids:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You do this process twice, you should make a function that does this. Don't repeat yourself 😉

Comment threadbot/cogs/snakes.py
DEBUG = environ.get('SNAKES_DEBUG', None)
dprint = print if DEBUG else lambda *a, **k: None # -1 index is used for easy temp debug hardcode

SNEK_FACTS = json.loads(open('bot/cogs/facts.json', 'r', encoding='utf-8').read())

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You never close this file, or the snek,picklebd file.

Comment threadbot/cogs/snakes.py
class NoGuessError(Exception):
def __init__(self, message='', debugdata=None):
self.message = message
self.debugdata = debugdata

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is python, let's use _ where there should be a space:

classNoGuessError(Exception):
def__init__(self, message='', debug_data=None):
self.message=messageself.debug_data=debug_data

Comment threadbot/cogs/snakes.py
'💀💀': 'Venomous, harmful',
'💀💀💀': 'Venomous, dangerous',
'💀💀💀💀': 'Venomous, very dangerous',
'💀💀💀💀💀': 'Venomous, extremely dangerous.'

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Could this be moved into a json file, that's already loaded?

Comment threadbot/cogs/snakes.py
"Tumblr_m2g660GN9z1qcwgmzo1_1280.png/revision/latest?cb=20120611212456"
scient = 'Chironex fleckeri'
length = '9.8 ft'
custom = True

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

These 3 sneks fall under the same issue as the danger dict. Move this data into a json file!

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@katlyn@gdude2002@lemonsaurus@Zwork101@silicWulf