Skip to content

Conversation

@meatsnails
Copy link
Collaborator

@meatsnails meatsnails commented Aug 29, 2024

im nto a fan of git.
previous pr: #479

Summary by Sourcery

Add a new Bookmarks feature to the Discord bot, enabling users to bookmark messages by reacting with an emoji. Update the README to reflect changes in the contributors' section.

New Features:

  • Introduce a new Bookmarks feature that allows users to bookmark messages in Discord by reacting with a specific emoji. The feature sends a direct message to the user with the bookmarked message details, including the author, content, and any attachments.

Documentation:

  • Update the README.md file to modify the contributors' section, rearranging the order of contributors and updating their details.

@sourcery-ai
Copy link
Contributor

sourcery-ai bot commented Aug 29, 2024

Reviewer's Guide by Sourcery

This pull request implements a new bookmarking feature for the Discord bot. The changes include updating the README.md file to modify the contributors list and adding a new Python file for the bookmarks functionality.

File-Level Changes

Change Details Files
Implemented a new bookmarking feature for the Discord bot
  • Created a new Bookmarks cog class
  • Implemented an event listener for the '🔖' reaction
  • Added logic to send bookmarked messages as DMs to users
  • Included error handling for various scenarios
  • Added logging for error cases
tux/cogs/utility/bookmarks.py
Updated the contributors list in the README
  • Removed two contributors (0x4248 and wlinator) from their original positions
  • Added targzballs as a new contributor
  • Rearranged the order of contributors in the table
README.md

Tips
  • Trigger a new Sourcery review by commenting @sourcery-ai review on the pull request.
  • Continue your discussion with Sourcery by replying directly to review comments.
  • You can change your review settings at any time by accessing your dashboard:
    • Enable or disable the Sourcery-generated pull request summary or reviewer's guide;
    • Change the review language;
  • You can always contact us if you have any questions or feedback.

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @targzballs - I've reviewed your changes and they look great!

Here's what I looked at during the review
  • 🟡 General issues: 3 issues found
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

self.bot = bot

@commands.Cog.listener()
async def on_raw_reaction_add(self, payload: discord.RawReactionActionEvent) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: Consider refactoring the method to reduce nesting and improve readability

The current structure with nested try-except blocks can be hard to follow. Consider extracting some logic into separate methods, such as fetching the channel and message, and sending the bookmark.

    async def on_raw_reaction_add(self, payload: discord.RawReactionActionEvent) -> None:
        if str(payload.emoji) != "🔖":
            return

        try:
            await self._handle_bookmark(payload)
        except Exception as e:
            logging.error(f"Error handling bookmark: {e}")

try:
channel = self.bot.get_channel(payload.channel_id)
if channel is None:
logger.error("Channel not found")
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: Use appropriate log levels for different types of errors

Consider using logger.warning for less critical issues (like 'Channel not found') and reserve logger.error for more severe problems. This will help in better error categorization and debugging.

Suggested change
logger.error("Channel not found")
logger.warning("Channel not found")


embed = EmbedCreator.create_info_embed(
title="Message Bookmarked",
description=f"> {message.content}",
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: Handle cases where message content might be empty

Add a check for empty message content and provide a default value or alternative text when the content is empty.

Suggested change
description=f"> {message.content}",
description=f"> {message.content if message.content else '[No message content]'}",

refactor(bookmarks.py): move bookmarks from utility to services for better organization
fix(bookmarks.py): improve error handling and logging in bookmarks service
chore(bookmarks.py): remove old bookmarks utility after successful refactor
@kzndotsh kzndotsh enabled auto-merge August 29, 2024 07:57
@kzndotsh kzndotsh self-assigned this Aug 29, 2024
@kzndotsh kzndotsh disabled auto-merge August 29, 2024 08:02
@kzndotsh kzndotsh merged commit a57dbdf into allthingslinux:main Aug 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants