Team 16 #9
Team 16 #9
Conversation
Pipfile
Outdated
| aiohttp = "*" | ||
| aiodns = "*" | ||
| bs4 = "*" | ||
| html2text = "*" |
There was a problem hiding this comment.
You have edited the Pipfile directly, and in a way that will break discord.py. Please revert your Pipfile and read over the doc/ folder again.
There was a problem hiding this comment.
I don't believe we'd had this change to the Pipfile PR'd yet, because I don't remember deleting the aiohttp/websockets lines. But also -- doesn't discord.py itself pin these two deps in its own requirements.txt?
There was a problem hiding this comment.
It does pin aiohttp and websockets, but Pipenv doesn't pick up on it correctly if they aren't specified directly in the pipfile. You also can't just use the latest version of aiohttp, it's not compatible.
I make a PR this morning, maybe check that out.
There was a problem hiding this comment.
Gotcha, & just saw that! I'll fix all this as soon as I get home.
| self.secs_query = API + 'parse&prop=sections&page={}' | ||
| self.img_query = API + 'query&titles={}&prop=pageimages&pithumbsize=300' | ||
|
|
||
| async def get_snek(self, name: str = None) -> Dict[str, Any]: |
There was a problem hiding this comment.
Why did you remove the type hinting?
There was a problem hiding this comment.
Sorry. My partner was for a short while only able to use python 3.5 (and typing was added in 3.6) so I started removing hints preemptively for compatibility -- didn't take care to put them all back once he was able to get on 3.6, though.
bot/cogs/snakes.py
Outdated
| @command() | ||
| async def get(self, ctx: Context, name: str = None): | ||
| @commands.command() | ||
| async def get(self, ctx, name: str.title = None): |
There was a problem hiding this comment.
You removed the type hinting for ctx. This will break things. Also, you can't type hint str.title.
There was a problem hiding this comment.
huh, what would the Context typehint do and/or break? (are you folks running a type checker?)
The str.title typehint is a converter -- see Rapptz/discord.py#1064 (bad pr on my part but it's there) and the documentation on converters. Converting to str is in fact redundant as far as ext.commands is concerned.
[BUT: I've also just realized I don't even want to convert with str.title(), because Wikipedia articles are titled in This case. Oops]
| @@ -1,300 +0,0 @@ | |||
| { | |||
There was a problem hiding this comment.
This file does need to be in the repo.
| import re | ||
| from typing import Any, Dict | ||
|
|
||
| from discord.ext.commands import AutoShardedBot, Context, command |
There was a problem hiding this comment.
Why not keep it like this? Seems cleaner than what you did below
| from discord.ext import commands | ||
| from discord.ext.commands import Context | ||
|
|
||
| from .. import hardcoded |
There was a problem hiding this comment.
Do not use relative imports please.
| '&prop=pageimages|categories' | ||
| '&pithumbsize=300' | ||
| f"&cllimit=max&clcategories={'|'.join(hardcoded.categories)}" | ||
| ) |
There was a problem hiding this comment.
Maybe unindent this paren one level?
bot/cogs/snakes.py
Outdated
| name = await self.get_rand_snek() | ||
|
|
||
| async with self.session.get(self.base_query.format(name)) as pg_resp, \ | ||
| self.session.get(self.info_query.format(name)) as if_resp: # noqa |
There was a problem hiding this comment.
What error are you squelching here? Indentation?
supposedly
force-pushed
the
master
branch
5 times, most recently
from
5a015eb to
22d874c
Compare
With @ShawSumma