WIP: Support for snippets (#21) #85
Conversation
jamievlin
force-pushed
the
dev/jamievlin/snippets
branch
8 times, most recently
from
1f3dd10
to
98b2006
Compare
jamievlin
force-pushed
the
dev/jamievlin/snippets
branch
from
98b2006
to
4144380
Compare
jamievlin
force-pushed
the
dev/jamievlin/snippets
branch
from
4144380
to
48c343d
Compare
jamievlin
force-pushed
the
dev/jamievlin/snippets
branch
from
48c343d
to
878835b
Compare
|
There was a problem hiding this comment.
@jamievlin First pass on reviewing snippets feature. Make sure to follow through with the suggestions at some point in the future.
bot/cogs/snippets.py
Outdated
| """ | ||
|
|
||
| def __init__(self, bot: Rodhaj): | ||
| self._bot = bot |
There was a problem hiding this comment.
| self._bot = bot | |
| self.bot = bot |
I know that you might not agree with me here, but generally the bot attr of an cog is left public by custom. This is generally how it's done.
bot/cogs/snippets.py
Outdated
|
|
||
| class Snippets(commands.Cog): | ||
| """ | ||
| Cog for snippet-related commands (#21) |
There was a problem hiding this comment.
| Cog for snippet-related commands (#21) | |
| Commands pertaining to snippets for Rodhaj |
Generally you can shrink this to something more descriptive. Try to not include the PR number as most users would be more confused by it
bot/cogs/snippets.py
Outdated
|
|
||
| @commands.guild_only() | ||
| @commands.group(name="snippet") | ||
| async def snippet_cmd(self, ctx: GuildContext): |
There was a problem hiding this comment.
| async def snippet_cmd(self, ctx: GuildContext): | |
| async def snippet(self, ctx: GuildContext) -> None: |
This is not documented but the convention for this project is to have each command function have the most simplest name. Thus, we can remove cmd out of the function name and just make it much more simpler.
bot/cogs/snippets.py
Outdated
| await ctx.send("placeholder for base command") | ||
|
|
||
| @commands.guild_only() | ||
| @snippet_cmd.command() |
There was a problem hiding this comment.
| @snippet_cmd.command() | |
| @snippet.command() |
Adjusting the decorator to align with the parent function. Make sure to do this for the rest of the grouped commands
| """ | ||
| result = await self._bot.pool.fetchrow(query, ctx.guild.id, name) | ||
| if result is None: | ||
| await ctx.reply( |
There was a problem hiding this comment.
| await ctx.reply( | |
| await ctx.send(...) |
I would strongly recommend not using commands.Context.reply in this case. It sends an unnecessary ping to the user, and is generally considered extremely annoying and bad practice. Instead, replace it with commands.Context.send()
| description=f"Snippet `{name}` was not found and " | ||
| + "hence was not deleted.", | ||
| ), | ||
| ephemeral=True, |
There was a problem hiding this comment.
The ephemeral keyword argument only works on interactions. It doesn't really make sense to have it enabled when in regular prefixed messages, that this message can be seen by everyone
| result = await self._bot.pool.fetchrow(query, ctx.guild.id, name) | ||
| if result is None: | ||
| await ctx.reply( | ||
| embed=discord.Embed( |
There was a problem hiding this comment.
I would strongly recommend replacing these embeds with just an message. It isn't necessary to do this as most users will find an jarring differences compared to the rest of the codebase
| @snippet_cmd.command() | ||
| async def edit(self, ctx: GuildContext, name: str, content: Optional[str]): | ||
| if content is None: | ||
| await self.edit_prompt_user(ctx, name) |
There was a problem hiding this comment.
If you have an function and it's only used once, then it would be better to contain the logic within the command function body instead.
| @commands.guild_only() | ||
| @snippet_cmd.command() | ||
| async def show(self, ctx: GuildContext, name: str): | ||
| query = """ |
There was a problem hiding this comment.
The rest of the code should follow the suggestions that are noted above
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
|
Summary
Add support for snippets
Closes #21
Types of changes
What types of changes does your code introduce to Rodhaj
Put an
xin the boxes that applyChecklist
Put an
xin the boxes that apply