-
Notifications
You must be signed in to change notification settings - Fork 9
Refactor multiple cogs to utilize a new architecture. #12
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
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
...omplexity and improving maintainability. Consolidate error handling and embed management across commands, enhancing user feedback and experience. Introduce centralized logging for image generation processes and streamline image request handling with the new ImageRequestBuilder.
...e. Update file listing logic in fs.py to exclude base_command_cog.py from Python file filtering, enhancing functionality.
...ation URL for consistency. Modify cross_pollinate_cog.py to include validate_prompt import for enhanced prompt validation.
Reviewer's GuideThis PR refactors the bot’s command cogs and utilities to a new, layered architecture by introducing a BaseCommandCog for shared logic, a unified Views system for UI components, and an ImageRequestBuilder for centralized URL construction, dramatically reducing duplication and improving maintainability. Sequence Diagram: Refactored Command Execution FlowsequenceDiagram
actor User
participant ImagineCmd as "/pollinate"
participant ImagineCog as Imagine (Cog)
participant BaseCog as BaseCommandCog
participant ImgGenUtils as image_gen_utils
participant ImgReqBuilder as ImageRequestBuilder
participant DiscordAPI as Discord API
participant ExternalImageAPI as Image API
User->>DiscordAPI: Executes /pollinate command
DiscordAPI->>ImagineCog: imagine_command(interaction, prompt, ...)
ImagineCog->>BaseCog: self.log_generation_start(...)
ImagineCog->>ImgGenUtils: generate_image(prompt, ...)
activate ImgGenUtils
ImgGenUtils->>ImgReqBuilder: builder = ImageRequestBuilder.for_standard_generation(prompt)
ImgGenUtils->>ImgReqBuilder: builder.with_dimensions(...)
ImgGenUtils->>ImgReqBuilder: builder.build_request_data()
ImgReqBuilder-->>ImgGenUtils: request_data (incl. URL)
ImgGenUtils->>ExternalImageAPI: GET request_data.url
ExternalImageAPI-->>ImgGenUtils: image_data, metadata
deactivate ImgGenUtils
ImgGenUtils-->>ImagineCog: (dic, image_file)
ImagineCog->>BaseCog: self.log_generation_complete(...)
ImagineCog->>ImagineCog: embed = generate_pollinate_embed(...)
ImagineCog->>ImagineCog: view = self.get_view_class()()
ImagineCog->>BaseCog: self.send_response(interaction, embed, file=image_file, view=view)
BaseCog->>DiscordAPI: Followup response with embed, image, view
DiscordAPI-->>User: Displays image and buttons
Sequence Diagram: Refactored 'Edit' Button InteractionsequenceDiagram
actor User
participant ImagineViewInst as ImagineView
participant EditModal as EditImageModal
participant ImgGenUtils as image_gen_utils
participant ImgReqBuilder as ImageRequestBuilder
participant ExternalImageAPI as Image API
participant DiscordAPI as Discord API
User->>DiscordAPI: Clicks "Edit" button
DiscordAPI->>ImagineViewInst: edit(interaction)
ImagineViewInst->>DiscordAPI: interaction.response.send_modal(EditModal(...))
DiscordAPI-->>User: Shows EditImageModal
User->>DiscordAPI: Submits EditImageModal (with edit_prompt)
DiscordAPI->>EditModal: on_submit(interaction)
activate EditModal
EditModal->>ImgGenUtils: generate_cross_pollinate(prompt=edit_prompt.value, image_url=original_image_url, ...)
activate ImgGenUtils
ImgGenUtils->>ImgReqBuilder: builder = ImageRequestBuilder.for_cross_pollination(...)
ImgGenUtils->>ImgReqBuilder: builder.build_request_data()
ImgReqBuilder-->>ImgGenUtils: request_data (incl. URL)
ImgGenUtils->>ExternalImageAPI: GET request_data.url
ExternalImageAPI-->>ImgGenUtils: edited_image_data, metadata
deactivate ImgGenUtils
ImgGenUtils-->>EditModal: (dic, edited_image_file)
EditModal->>EditModal: embed = generate_cross_pollinate_embed(...)
EditModal->>DiscordAPI: interaction.followup.send(embed=embed, file=edited_image_file, view=CrossPollinateView())
deactivate EditModal
DiscordAPI-->>User: Displays edited image and new buttons
Class Diagram: Command Cog HierarchyclassDiagram
class BaseCommandCog {
+bot: commands.Bot
+command_name: str
+command_config: dict
+logger: Logger
+__init__(bot, command_name_in_config)
+cog_load()*
+handle_command_error(interaction, error, **context)*
+send_response(interaction, embed, file, ephemeral, view_instance)*
+get_view_class() View
#log_generation_start(model, dimensions, cached, action)
#log_generation_complete(model, dimensions, generation_time, cached, action)
}
class Imagine {
+__init__(bot)
+get_view_class() ImagineView
+imagine_command(interaction, prompt, ...)
}
class CrossPollinate {
+__init__(bot)
+get_view_class() CrossPollinateView
+cross_pollinate_command(interaction, prompt, image, ...)
}
class Beemoji {
+__init__(bot)
+get_view_class() BeemojiView
+beemoji_command(interaction, emoji1, emoji2, ...)
}
class MultiPollinate {
+__init__(bot)
+get_view_class() MultiPollinateView
+multiimagine_command(interaction, prompt, ...)
}
class RandomImage {
+__init__(bot)
+get_view_class() CrossPollinateView
+random_image_command(interaction, ...)
}
BaseCommandCog <|-- Imagine
BaseCommandCog <|-- CrossPollinate
BaseCommandCog <|-- Beemoji
BaseCommandCog <|-- MultiPollinate
BaseCommandCog <|-- RandomImage
Class Diagram: UI View HierarchyclassDiagram
class BaseImageView {
+timeout: Optional[float]
+__init__(timeout)
+setup_common_buttons()*
+interaction_check(interaction)* bool
+handle_delete_button(interaction)*
+handle_bookmark_button(interaction)*
+get_original_data(interaction)* dict
+get_prompt_from_embed(embed) str
#_get_bookmark_type() str
}
class ImagineView {
+setup_buttons()*
+regenerate(interaction)*
+edit(interaction)*
}
class CrossPollinateView {
+setup_buttons()*
+edit(interaction)*
}
class MultiPollinateView {
+image_count: int
+setup_buttons()*
+handle_upscale_button(interaction, index)*
}
class BeemojiView {
+emoji1_name: str
+emoji2_name: str
+generated_name: str
+setup_buttons()*
+edit(interaction)*
+add_to_server(interaction)*
+reduce_image_size(image_data, width, height) bytes
}
class EditImageModal {
+original_image_url: str
+original_prompt: str
+edit_prompt: TextInput
+__init__(original_image_url, original_prompt)
+on_submit(interaction)*
}
BaseImageView <|-- ImagineView
BaseImageView <|-- CrossPollinateView
BaseImageView <|-- MultiPollinateView
BaseImageView <|-- BeemojiView
ImagineView ..> EditImageModal : "uses"
CrossPollinateView ..> EditImageModal : "uses"
BeemojiView ..> EditImageModal : "uses for edit"
Class Diagram: Image Request ConstructionclassDiagram
class ImageRequest {
+prompt: Optional[str]
+width: int
+height: int
+model: Optional[str]
+safe: bool
+cached: bool
+nologo: bool
+enhance: bool
+private: bool
+negative: Optional[str]
+seed: Optional[str]
+image_url: Optional[str]
+to_dict() dict
}
class ImageRequestBuilder {
-base_url: str
-request: ImageRequest
+__init__(base_url)
+with_prompt(prompt) ImageRequestBuilder
+with_dimensions(width, height) ImageRequestBuilder
+with_model(model) ImageRequestBuilder
+with_source_image(image_url) ImageRequestBuilder
+build_url() str
+build_request_data() dict
+for_standard_generation(prompt, **kwargs)$ ImageRequestBuilder
+for_cross_pollination(prompt, image_url, **kwargs)$ ImageRequestBuilder
+for_random_generation(**kwargs)$ ImageRequestBuilder
}
class MultiImageRequestBuilder {
-base_request: ImageRequest
-models: list[str]
+__init__(base_request)
+with_base_prompt(prompt) MultiImageRequestBuilder
+build_requests() list[ImageRequestBuilder]
}
ImageRequestBuilder "1" *-- "1" ImageRequest : aggregates
MultiImageRequestBuilder "1" *-- "1" ImageRequest : aggregates
MultiImageRequestBuilder ..> ImageRequestBuilder : creates
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
@sourcery-ai
sourcery-ai
bot
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Zingzy - I've reviewed your changes - here's some feedback:
- BaseCommandCog.send_response ignores
fileandviewwhenephemeral=True, so ephemeral responses cannot include attachments or interactive buttons; consider including them for consistent UX. - You can DRY up your cog setup by using the provided
setup_base_coghelper in eachsetupfunction instead of manually callingbot.add_cogand logging. - The manual string concatenation in
ImageRequestBuilder.build_urlcan miss edge‐case escaping—consider usingurllib.parse.urlencodeto assemble query parameters more robustly.
Here's what I looked at during the review
- 🟡 General issues: 6 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 3 issues found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
return (request_data, image_file)
except aiohttp.ClientError as e:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (bug_risk): APIError is raised with only the string message, which may lose status code context.
Consider passing the status code to APIError if available, to preserve error context for downstream handlers.
else:
await response.edit(embeds=embeds, attachments=files)
# For multi-pollinate, we need to edit the response with the grid
view = MultiPollinateView(image_count=len(embeds))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: MultiPollinateView is instantiated with image_count, but get_view_class does not pass this parameter.
If get_view_class is used elsewhere without image_count, this could cause inconsistent behavior. Please standardize how MultiPollinateView is instantiated or clearly document the image_count requirement.
"""Handle button interactions."""
custom_id = interaction.data["custom_id"]
if custom_id.startswith("U"):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (bug_risk): Index parsing only handles single-digit custom_ids
Parsing only custom_id[1] fails for multi-digit indices. Use int(custom_id[1:]) to handle all cases correctly.
)
return
# Get the original image URL from the message embed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (bug_risk): Only checks the first embed for an image
Iterate through all embeds to locate the first one with an image, rather than assuming the first embed always contains it.
async def interaction_check(self, interaction: discord.Interaction) -> bool:
"""Handle button interactions."""
custom_id = interaction.data["custom_id"]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (bug_risk): Direct access to interaction.data['custom_id'] may throw KeyError
Use interaction.data.get('custom_id') or check if 'custom_id' exists before accessing it to prevent KeyError.
return (request_data, image_file)
except aiohttp.ClientError as e:
raise APIError(f"Network error occurred: {str(e)}")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (code-quality): Explicitly raise from a previous error (raise-from-previous-error)
Suggested change
raise APIError(f"Network error occurred: {str(e)}")
raise APIError(f"Network error occurred: {str(e)}")frome
return (request_data, image_file)
except aiohttp.ClientError as e:
raise APIError(f"Network error occurred: {str(e)}")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (code-quality): Explicitly raise from a previous error (raise-from-previous-error)
Suggested change
raise APIError(f"Network error occurred: {str(e)}")
raise APIError(f"Network error occurred: {str(e)}")frome
"Authorization": f"Bearer {config.api.api_key}",
}
try:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (code-quality): We've found these issues:
- Use named expression to simplify assignment and conditional (
use-named-expression) - Explicitly raise from a previous error (
raise-from-previous-error)
Comment on lines
+121
to
+124
# Add dimensions
params.append(f"width={self.request.width}")
params.append(f"height={self.request.height}")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (code-quality): Merge consecutive list appends into a single extend (merge-list-appends-into-extend)
Suggested change
# Add dimensions
params.append(f"width={self.request.width}")
params.append(f"height={self.request.height}")
params.extend((f"width={self.request.width}", f"height={self.request.height}"))
Comment on lines
+27
to
+33
# Find the prompt field (different commands use different field names)
prompt = "Unknown prompt"
for field in interaction_data.get("fields", []):
if field["name"] in ["Prompt", "Cross-Pollinate Prompt 🐝"]:
prompt = field["value"][3:-3] # Remove ``` code block markers
break
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (code-quality): Use the built-in function next instead of a for-loop (use-next)
Suggested change
# Find the prompt field (different commands use different field names)
prompt = "Unknown prompt"
for field in interaction_data.get("fields", []):
if field["name"] in ["Prompt", "Cross-Pollinate Prompt 🐝"]:
prompt = field["value"][3:-3] # Remove ``` code block markers
break
prompt = next(
(
field["value"][3:-3]
for field in interaction_data.get("fields", [])
if field["name"] in ["Prompt", "Cross-Pollinate Prompt 🐝"]
),
"Unknown prompt",
)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
Reducing code reusability and complexity and improving maintainability. Consolidate error handling and embed management across commands, enhancing user feedback and experience. Introduce centralized logging for image generation processes and streamline image request handling with the new
ImageRequestBuilderDescription
Brief description of the changes made in this PR.
Type of Change
Please delete options that are not relevant:
Testing
Please describe the tests that you ran to verify your changes:
uvx ruff formatuvx ruff check --fixChecklist
Summary by Sourcery
Refactor all core image command cogs to adopt a new extensible architecture: introduce a shared BaseCommandCog for common logic, leverage BaseImageView and command-specific views for UI buttons, and unify image request handling with ImageRequestBuilder. Consolidate error handling and logging, eliminate duplication, and add comprehensive documentation for the new design.
New Features:
Enhancements:
Documentation:
Chores: