added simple little before and after when editing a snippet (unfinished) #418
Conversation
Reviewer's Guide by SourceryThis pull request enhances the snippet editing functionality by adding a before and after comparison when a snippet is edited. The changes are implemented in the File-Level Changes
Tips
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @GabberBalls - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider optimizing by using the result of the first database query instead of querying twice for the same snippet.
- For longer snippets, consider truncating the content displayed in the confirmation message to avoid potentially lengthy outputs.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟡 Security: 1 issue found
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
tux/cogs/utility/snippets.py
Outdated
| return | ||
|
|
||
| name = args[0] | ||
| snippet = await self.db.get_snippet_by_name_and_guild_id(name, ctx.guild.id) |
There was a problem hiding this comment.
suggestion (performance): Consider retrieving the snippet once to improve performance
The snippet is currently retrieved twice from the database. Consider retrieving it once at the beginning of the function and using it for all subsequent operations. This would improve performance while maintaining code clarity.
| snippet = await self.db.get_snippet_by_name_and_guild_id(name, ctx.guild.id) | |
| snippet = await self.db.get_snippet_by_name_and_guild_id(name, ctx.guild.id) | |
| if snippet: | |
| previous_content = snippet.snippet_content | |
| content = " ".join(args[1:]) |
tux/cogs/utility/snippets.py
Outdated
| await ctx.send("Snippet Edited.", delete_after=30, ephemeral=True) # Correct indentation | ||
| logger.info(f"{ctx.author} Edited a snippet with the name {name} and content {content}.") # Correct indentation | ||
| await ctx.send(f"Snippet Edited. {previous_content} -> {content}", delete_after=30, ephemeral=True) | ||
| logger.info(f"{ctx.author} Edited a snippet with the name {name} and content {content}.") |
There was a problem hiding this comment.
suggestion: Include previous content in log message for better auditing
For improved auditing, consider including both the previous and new content in the log message. This would provide a more complete record of the changes made to snippets.
| logger.info(f"{ctx.author} Edited a snippet with the name {name} and content {content}.") | |
| logger.info(f"{ctx.author} Edited snippet '{name}'. Old content: '{previous_content}', New content: '{content}'") |
tux/cogs/utility/snippets.py
Outdated
|
|
||
| await ctx.send("Snippet Edited.", delete_after=30, ephemeral=True) # Correct indentation | ||
| logger.info(f"{ctx.author} Edited a snippet with the name {name} and content {content}.") # Correct indentation | ||
| await ctx.send(f"Snippet Edited. {previous_content} -> {content}", delete_after=30, ephemeral=True) |
There was a problem hiding this comment.
🚨 suggestion (security): Consider the security implications of displaying full snippet content
While showing the previous and new content is informative, it could potentially expose sensitive information. Consider using a more generic message or truncating the content if it's too long to balance informativeness with security concerns.
| await ctx.send(f"Snippet Edited. {previous_content} -> {content}", delete_after=30, ephemeral=True) | |
| truncated_prev = (previous_content[:50] + '...') if len(previous_content) > 50 else previous_content | |
| truncated_new = (content[:50] + '...') if len(content) > 50 else content | |
| await ctx.send(f"Snippet Edited. '{truncated_prev}' -> '{truncated_new}'", delete_after=30, ephemeral=True) |
|
i'll be changing this to embeds later on but currently this is what i doing for now |
electron271
left a comment
electron271
left a comment
There was a problem hiding this comment.
very spammy with large snippets, at least truncate each to 50 chars
added that for now |
as kaizen said ill prob add buttons to view the before and after once i get to adding embeding |
meatsnails
changed the title
|
kaizen wants me to add an embed and buttons for viewing the before and after before merging it so i marked it as unfinished |
|
ok |
2 participants
Summary by Sourcery
Enhance the snippet editing functionality by showing the previous content alongside the new content in the confirmation message when a snippet is edited.
Enhancements: