Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Support for snippets (#21) #85

Draft
wants to merge 18 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
128 changes: 128 additions & 0 deletions bot/cogs/snippets.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,128 @@
from typing import Optional

import discord
from discord.ext import commands
from libs.utils import GuildContext
from rodhaj import Rodhaj


class Snippets(commands.Cog):
"""
Cog for snippet-related commands (#21)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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

"""

def __init__(self, bot: Rodhaj):
self._bot = bot
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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.


@commands.guild_only()
@commands.group(name="snippet")
async def snippet_cmd(self, ctx: GuildContext):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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.

if ctx.invoked_subcommand is None:
await ctx.send("placeholder for base command")

@commands.guild_only()
@snippet_cmd.command()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@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

async def remove(self, ctx: GuildContext, name: str):
query = """
DELETE FROM snippets
WHERE guild_id = $1 AND name = $2
RETURNING name
Copy link
Member

Choose a reason for hiding this comment

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

I'm unsure of why we need to return the name column here. It would be more ideal to return the row's id.

"""
result = await self._bot.pool.fetchrow(query, ctx.guild.id, name)
if result is None:
await ctx.reply(
Copy link
Member

@No767 No767 May 30, 2024

Choose a reason for hiding this comment

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

Suggested change
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()

embed=discord.Embed(
Copy link
Member

Choose a reason for hiding this comment

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

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

title="Deletion failed",
colour=discord.Colour.red(),
description=f"Snippet `{name}` was not found and "
+ "hence was not deleted.",
),
ephemeral=True,
Copy link
Member

Choose a reason for hiding this comment

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

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

)
else:
Copy link
Member

Choose a reason for hiding this comment

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

For future reference, there isn't an need for an else statement here. It would be ideal to terminate it using an return keyword instead. An good example would be here

await ctx.reply(
embed=discord.Embed(
title="Deletion successful",
colour=discord.Colour.green(),
description=f"Snippet `{name}` was deleted successfully",
),
ephemeral=True,
)

@commands.guild_only()
@snippet_cmd.command()
async def new(self, ctx, *args):
await ctx.send("placeholder for snippet new")

@commands.guild_only()
@snippet_cmd.command()
async def show(self, ctx: GuildContext, name: str):
query = """
Copy link
Member

Choose a reason for hiding this comment

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

The rest of the code should follow the suggestions that are noted above

SELECT content FROM snippets
WHERE guild_id = $1 AND name = $2
"""
data = await self._bot.pool.fetchrow(query, ctx.guild.id, name)
if data is None:
ret_embed = discord.Embed(
title="Oops...",
colour=discord.Colour.red(),
description=f"The snippet `{name}` was not found. "
+ "To create a new snippet with this name, "
+ f"please run `snippet create {name} <content>`",
)
await ctx.reply(embed=ret_embed, ephemeral=True)
else:
ret_data = discord.Embed(
title=f"Snippet information for `{name}`",
colour=discord.Colour.green(),
description=data[0],
)
await ctx.reply(embed=ret_data, ephemeral=True)

@commands.guild_only()
@snippet_cmd.command()
async def list(self, ctx, *args):
await ctx.send("placeholder for snippet list")

async def edit_prompt_user(self, ctx: GuildContext, name: str):
raise NotImplementedError("TODO: Add prompt for editing snippet.")

@commands.guild_only()
@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)
Copy link
Member

Choose a reason for hiding this comment

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

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.

return
query = """
UPDATE snippets
SET content = $3
WHERE guild_id = $1 AND name = $2
RETURNING name
"""

result = await self._bot.pool.fetchrow(query, ctx.guild.id, name, content)
if result is None:
await ctx.reply(
embed=discord.Embed(
title="Oops...",
colour=discord.Colour.red(),
description=f"Cannot edit snippet `{name}` as there is no such "
+ "snippet. To create a new snippet with the corresponding "
+ f"name, please run `snippet new {name} <snippet text>`.",
),
ephemeral=True,
)
else:
await ctx.reply(
embed=discord.Embed(
title="Snippet changed",
colour=discord.Colour.green(),
description=f"The contents of snippet {result[0]} has been "
+ f"changed to \n\n{content}",
),
ephemeral=True,
)


async def setup(bot: Rodhaj):
await bot.add_cog(Snippets(bot))
12 changes: 12 additions & 0 deletions bot/migrations/V5__add_table_for_snippets.sql
No767 marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
-- Revision Version: V5
-- Revises: V4
-- Creation Date: 2024-03-10 05:51:39.252162 UTC
-- Reason: add table for snippets

CREATE TABLE IF NOT EXISTS snippets
(
guild_id bigint NOT NULL,
name VARCHAR(100),
content TEXT,
PRIMARY KEY (guild_id, name)
);