-
-
Notifications
You must be signed in to change notification settings - Fork 16
Team 13 #10
base: master
Are you sure you want to change the base?
Team 13 #10
Conversation
bot/cogs/snakes.py
Outdated
|
|
||
| @command() | ||
| async def get(self, ctx: Context, name: str = None): | ||
| async def get(self, ctx: Context, name: str = "python"): |
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.
No, this should default to None. The task requires a random snake if the parameter is left out.
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 - just noticed this - thanks!
lemonsaurus
left a comment
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.
Keep up the good work!
bot/cogs/snakes.py
Outdated
| :param name: Optional, the name of the snake to get information for - omit for a random snake | ||
| """ | ||
| embed = discord.Embed(color=0x3E885B) | ||
| if name == "python": |
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 be case insensitive.
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.
Thanks - so when I change line 73 to if name.lower() == "python":, I get an error about comparing NoneTypes since the default name is None (see the method declaration). How should I get around this? Should I do an "if None" check before the python special case or should I do something else?
(discord.ext.commands.errors.CommandInvokeError: Command raised an exception: AttributeError: 'NoneType' object has no attribute 'lower' is the error for reference)
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.
if name and name.lower() == "python":would work for me.
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 - looks good, thanks
bot/cogs/snakes.py
Outdated
| :return: the full JSON from the search API | ||
| """ | ||
| url = "https://api.unsplash.com/search/photos?client_id" \ | ||
| "={0}&query={1}+snake".format(os.environ.get("UNSPLASH_CLIENT_ID"), snake_name) |
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 string is a bit messy. prefer implicit string joins inside paranthesis and f-strings to \ line continuation and formats.
client_id = os.environ.get("UNSPLASH_CLIENT_ID")
url = (
"https://api.unsplash.com/search/photos?client_id"
f"={client_id}&query={snake_name}+snake"
)
bot/cogs/snakes.py
Outdated
| :return: A dict containing information on a snake | ||
| """ | ||
|
|
||
| async def get_snek_qwant_json(self, snake_name): |
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.
snake_name should be type hinted, as should the return type.
bot/cogs/snakes.py
Outdated
| response = await response.read() | ||
| return json.loads(response.decode("utf-8")) | ||
|
|
||
| async def get_snek_image(self, name): |
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.
name should be type hinted, as should return type.
bot/cogs/snakes.py
Outdated
| embed.add_field( | ||
| name="Python (programming language)", | ||
| value="*Guido van Rossum*\n\n" | ||
| "This language is neither dangerous nor venomous and be found in software globally", |
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.
Unless you're a pirate, this should be "and can be found".
bot/cogs/snakes.py
Outdated
| # handle Python special case | ||
| embed.add_field( | ||
| name="Python (programming language)", | ||
| value="*Guido van Rossum*\n\n" |
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.
multiline implicit string joins should be wrapped inside paranthesis, so that it's clear that they're one thing. This is a bit too implicit.
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.
Can you explain this? I'm not super clear as to what you mean...
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.
value=(
"*Guido van Rossum*\n\n"
"This language is neither dangerous nor venomous and be found in software globally"
),this is one way.
or if you prefer it, this is acceptable too
value=("*Guido van Rossum*\n\n"
"This language is neither dangerous nor venomous and be found in software globally"),…sponse, just return the embed
[UPDATE] Ignored a silly rule from flake8.
Adding W503 and E226 to flake8 ignore list.
Use Qwant images instead of Unsplash
gdude2002
left a comment
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 wonder why prith didn't push anything except for one commit
| @@ -1,15 +1,21 @@ | |||
| [[source]] | |||
|
|
|||
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's loads of extra spaces in this file now. Did you edit it by hand?
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.
nope - this happened automatically when I installed the titlecase package
| "sha256": "d797e580ddcddc99bf058109ab0306ad584c2902752a3d4076ba713fdc580fb7" | ||
| "sha256": "2cca231e2bd4b5fdaa06fd33b68143231524f2855847ebf4cc497462e135fdf2" | ||
| }, | ||
| "host-environment-markers": { |
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.
Huh. Where'd this come from? Also, why did you sort all the hashes in this file?
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 happened automatically when I installed the titlecase package
bot/cogs/resources/facts.json
Outdated
| "Snakes kill over 40,000 people a year—though, with unreported incidents, the total may be over 100,000. About half of these deaths are in India.", | ||
| "Some members of the U.S. Army Special Forces are taught to kill and eat snakes during their survival training, which has earned them the nickname “Snake Eaters.”" | ||
| ] | ||
| } No newline at end of file |
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.
Missing a newline at the end of the file.
bot/cogs/snakes.py
Outdated
| async with aiohttp.ClientSession() as session: | ||
| async with session.get(url, headers=head) as response: | ||
| response = await response.read() | ||
| return json.loads(response.decode("utf-8")) |
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.
Couldn't you just use return await response.json()?
| :param name: Optional, the name of the snake to get information for - omit for a random snake | ||
| :return: A dict containing information on a snake | ||
| """ | ||
| base_url = "https://protected-reef-75100.herokuapp.com/" |
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.
Huh, that's an odd decision.
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.
Can we get more info on what you're doing 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.
Hey @gdude2002 , that's where we hosted our snake API. Here's the link to the repo. This is where most of my commits are :)
bot/cogs/snakes.py
Outdated
| # Any additional commands can be placed here. Be creative, but keep it to a reasonable amount! | ||
|
|
||
| @command(aliases=["f"]) | ||
| async def fact(self, ctx: Context): |
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 would just like to point out that you put a ton of work into the get command, and not much into your extra stuff.
The marking is heavily weighted towards the extra creative stuff. But you still have 12 hours!
Add video command
Add cat input for GIF
|
You code is failing to lint. Please see Travis for more information. |
Add Zen of Python
this will hold our submissions for the code jam - thanks for setting this up!