refactor(treewide): reorder statements to improve speed #953
+31
−34
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
Reviewer's GuideThis PR optimizes the cog loader and command handlers by caching Sentry initialization to eliminate redundant calls, and by streamlining conditional logic in the avatar and xkcd commands to simplify control flow and improve performance. Sequence diagram for optimized Sentry initialization in cog loadersequenceDiagram
participant CogLoader
participant SENTRY_INITIALIZED
participant sentry_sdk
participant current_span
CogLoader->>SENTRY_INITIALIZED: Check if Sentry is initialized (cached)
alt SENTRY_INITIALIZED is True
CogLoader->>sentry_sdk: get_current_span()
alt current_span exists
CogLoader->>current_span: set_tag/set_data
end
end
Sequence diagram for refactored avatar command handlersequenceDiagram
participant User as actor
participant AvatarCommandHandler
participant discord.Interaction
User->>AvatarCommandHandler: send_avatar(source, member)
alt source is discord.Interaction and member exists
AvatarCommandHandler->>discord.Interaction: response.send_message(files=files)
else member has no avatar
AvatarCommandHandler->>discord.Interaction: response.send_message(content=message, ephemeral=True, delete_after=30)
else source is commands.Context
AvatarCommandHandler->>AvatarCommandHandler: member = MemberConverter().convert(source, author.id)
end
Sequence diagram for refactored xkcd command handlersequenceDiagram
participant User as actor
participant XKCDCommandHandler
participant commands.Context
User->>XKCDCommandHandler: xkcd(ctx, comic_id)
alt comic_id is not provided
XKCDCommandHandler->>commands.Context: send_help("xkcd")
XKCDCommandHandler-->>User: return
else comic_id is provided
XKCDCommandHandler->>XKCDCommandHandler: specific(ctx, comic_id)
end
Class diagram for Sentry initialization optimization in cog loaderclassDiagram
class CogLoader {
+load_times: dict
+async _load_single_cog(path: Path)
+async _load_cog_group(cogs: Sequence[Path])
+async _process_single_file(path: Path)
+async _process_directory(path: Path)
+async load_cogs(path: Path)
+async load_cogs_from_folder(folder_name: str)
+static async setup(bot: commands.Bot)
}
class sentry_sdk {
+is_initialized()
+get_current_span()
}
CogLoader ..> sentry_sdk : uses
class SENTRY_INITIALIZED {
+bool
}
CogLoader ..> SENTRY_INITIALIZED : uses
Class diagram for refactored avatar command handlerclassDiagram
class AvatarCommandHandler {
+async send_avatar(source, member)
+async create_avatar_file(avatar)
}
class discord.Interaction {
+response.send_message()
}
class commands.Context {
+author
+reply()
}
AvatarCommandHandler ..> discord.Interaction : interacts
AvatarCommandHandler ..> commands.Context : interacts
Class diagram for refactored xkcd command handlerclassDiagram
class XKCDCommandHandler {
+async xkcd(ctx, comic_id)
+async specific(ctx, comic_id)
}
class commands.Context {
+send_help()
}
XKCDCommandHandler ..> commands.Context : interacts
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @amadaluzia - I've reviewed your changes - here's some feedback:
- Caching sentry_sdk.is_initialized() in IS_INITIALIZED at import time assumes Sentry is initialized before this module loads—consider a lazy check or helper function to avoid stale values if initialization happens later.
- There’s a lot of repeated
if IS_INITIALIZED and (current_span := get_current_span())boilerplate—extracting that into a small helper would reduce repetition and make future span calls easier to maintain. - The avatar refactor removes the fallback that converted the context author into a Member when no member was passed; double-check that you’re not losing intended behavior for context commands without explicit members.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Caching sentry_sdk.is_initialized() in IS_INITIALIZED at import time assumes Sentry is initialized before this module loads—consider a lazy check or helper function to avoid stale values if initialization happens later.
- There’s a lot of repeated `if IS_INITIALIZED and (current_span := get_current_span())` boilerplate—extracting that into a small helper would reduce repetition and make future span calls easier to maintain.
- The avatar refactor removes the fallback that converted the context author into a Member when no member was passed; double-check that you’re not losing intended behavior for context commands without explicit members.
## Individual Comments
### Comment 1
<location> `tux/cogs/fun/xkcd.py:38` </location>
<code_context>
The ID of the xkcd comic to search for.
"""
- if comic_id:
- await self.specific(ctx, comic_id)
- else:
+ if not comic_id:
await ctx.send_help("xkcd")
+ return
</code_context>
<issue_to_address>
The logic for handling comic_id=0 may have changed unintentionally.
The updated condition causes comic_id=0 to trigger the help message instead of fetching comic 0. If 0 is valid, use 'if comic_id is not None' to avoid this regression.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @amadaluzia - I've reviewed your changes - here's some feedback:
- You have a lot of repeated
if IS_INITIALIZED and get_current_span()checks—consider extracting that pattern into a helper or decorator to reduce duplication and improve readability. - Caching
sentry_sdk.is_initialized()at import time can become stale if Sentry is initialized later—confirm that initialization always happens before this module loads or revert to a dynamic check. - The
avatarcommand now only usessource.responsefor sending, so verify that non-Interaction (Context) invocations still behave as expected or explicitly handle both cases.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- You have a lot of repeated `if IS_INITIALIZED and get_current_span()` checks—consider extracting that pattern into a helper or decorator to reduce duplication and improve readability.
- Caching `sentry_sdk.is_initialized()` at import time can become stale if Sentry is initialized later—confirm that initialization always happens before this module loads or revert to a dynamic check.
- The `avatar` command now only uses `source.response` for sending, so verify that non-Interaction (Context) invocations still behave as expected or explicitly handle both cases.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #953 +/- ##
========================================
+ Coverage 9.63% 9.75% +0.12%
========================================
Files 123 123
Lines 10414 10412 -2
Branches 1279 1277 -2
========================================
+ Hits 1003 1016 +13
+ Misses 9307 9280 -27
- Partials 104 116 +12
*This pull request uses carry forward flags. Click here to find out more.
☔ View full report in Codecov by Sentry. |
anemoijereja-eden
requested changes
Jul 17, 2025
anemoijereja-eden
force-pushed
the
optimising
branch
from
abc5368 to
a50dbb3
Compare
There was a problem hiding this comment.
Hey @amadaluzia - I've reviewed your changes - here's some feedback:
- Using a module-level IS_INITIALIZED constant can be problematic if sentry_sdk is initialized later at runtime; consider a lazy check or ensuring sentry is always set up before import.
- send_avatar refactor only handles Interaction sources in the primary branch and falls through for Context sources, which could break direct avatar commands in ctx—please verify Context handling remains correct.
- A lot of boilerplate around checking IS_INITIALIZED and fetching current_span could be DRYed up with a helper or context manager to further clean this code.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Using a module-level IS_INITIALIZED constant can be problematic if sentry_sdk is initialized later at runtime; consider a lazy check or ensuring sentry is always set up before import.
- send_avatar refactor only handles Interaction sources in the primary branch and falls through for Context sources, which could break direct avatar commands in ctx—please verify Context handling remains correct.
- A lot of boilerplate around checking IS_INITIALIZED and fetching current_span could be DRYed up with a helper or context manager to further clean this code.
## Individual Comments
### Comment 1
<location> `tux/cogs/fun/xkcd.py:38` </location>
<code_context>
The ID of the xkcd comic to search for.
"""
- if comic_id:
- await self.specific(ctx, comic_id)
- else:
+ if not comic_id:
await ctx.send_help("xkcd")
+ return
</code_context>
<issue_to_address>
The logic for handling comic_id has been inverted, which may affect behavior for comic_id=0.
The updated logic prevents access to comic 0 if it's valid. Explicitly check for None to allow comic_id=0.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -35,10 +35,11 @@ | |||
| The ID of the xkcd comic to search for. | |||
| """ | |||
|
|
|||
| if comic_id: | |||
There was a problem hiding this comment.
issue (bug_risk): The logic for handling comic_id has been inverted, which may affect behavior for comic_id=0.
The updated logic prevents access to comic 0 if it's valid. Explicitly check for None to allow comic_id=0.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @amadaluzia - I've reviewed your changes - here's some feedback:
- Using a module-level SENTRY_INITIALIZED flag means that if sentry_sdk.is_initialized() returns false at import time the flag will never update; consider lazily calling sentry_sdk.is_initialized() or refreshing the flag at runtime.
- In the xkcd command, use an explicit 'if comic_id is None' check instead of falsy 'if not comic_id' to avoid treating a comic_id of 0 as missing.
- The refactored avatar handler no longer sends replies for Context sources after converting the member—reintroduce or unify Context support so non-Interaction invocations still produce a response.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Using a module-level SENTRY_INITIALIZED flag means that if sentry_sdk.is_initialized() returns false at import time the flag will never update; consider lazily calling sentry_sdk.is_initialized() or refreshing the flag at runtime.
- In the xkcd command, use an explicit 'if comic_id is None' check instead of falsy 'if not comic_id' to avoid treating a comic_id of 0 as missing.
- The refactored avatar handler no longer sends replies for Context sources after converting the member—reintroduce or unify Context support so non-Interaction invocations still produce a response.
## Individual Comments
### Comment 1
<location> `tux/cogs/info/avatar.py:78` </location>
<code_context>
member : discord.Member
The member to get the avatar of.
"""
- if member is not None:
+ if isinstance(source, discord.Interaction) and member is not None:
guild_avatar = member.guild_avatar.url if member.guild_avatar else None
global_avatar = member.avatar.url if member.avatar else None
</code_context>
<issue_to_address>
The function now only handles discord.Interaction sources for members, potentially breaking support for other source types.
If this change is intentional, please document or enforce the restriction. Otherwise, it may break support for non-Interaction sources.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -75,22 +75,16 @@ | |||
| member : discord.Member | |||
| The member to get the avatar of. | |||
| """ | |||
| if member is not None: | |||
There was a problem hiding this comment.
issue (bug_risk): The function now only handles discord.Interaction sources for members, potentially breaking support for other source types.
If this change is intentional, please document or enforce the restriction. Otherwise, it may break support for non-Interaction sources.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @amadaluzia - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `tux/cog_loader.py:17` </location>
<code_context>
from tux.utils.config import CONFIG
from tux.utils.sentry import safe_set_name, span, start_span, transaction
+SENTRY_INITIALIZED: bool = sentry_sdk.is_initialized()
+
</code_context>
<issue_to_address>
SENTRY_INITIALIZED is set at import time, which may not reflect runtime state.
If sentry_sdk may be initialized after import, caching its state could cause incorrect behavior. Consider calling sentry_sdk.is_initialized() directly unless initialization is guaranteed to be static.
</issue_to_address>
### Comment 2
<location> `tux/cogs/info/avatar.py:85` </location>
<code_context>
if files:
- if isinstance(source, discord.Interaction):
- await source.response.send_message(files=files)
- else:
- await source.reply(files=files)
</code_context>
<issue_to_address>
No fallback for non-Interaction sources when sending avatar files.
If source can be something other than discord.Interaction, add a fallback or raise a clear error to avoid AttributeError.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -14,6 +14,8 @@ | |||
| from tux.utils.config import CONFIG | |||
| from tux.utils.sentry import safe_set_name, span, start_span, transaction | |||
|
|
|||
| SENTRY_INITIALIZED: bool = sentry_sdk.is_initialized() | |||
There was a problem hiding this comment.
issue (bug_risk): SENTRY_INITIALIZED is set at import time, which may not reflect runtime state.
If sentry_sdk may be initialized after import, caching its state could cause incorrect behavior. Consider calling sentry_sdk.is_initialized() directly unless initialization is guaranteed to be static.
| await source.response.send_message(files=files) | ||
| else: | ||
| await source.reply(files=files) | ||
| await source.response.send_message(files=files) | ||
| else: |
There was a problem hiding this comment.
issue (bug_risk): No fallback for non-Interaction sources when sending avatar files.
If source can be something other than discord.Interaction, add a fallback or raise a clear error to avoid AttributeError.
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.
Description
This refactors the tree to optimise the cog loader, the
avatarcommand and thexkcdcommand with reordered if statements, and storing variables to remove unnecessary function calls. I think this is clean enough at the moment for me to write this as a PR. There will of course still be some unoptimal things I have missed but I cannot seem to find any that wouldn't conflict with #853.Guidelines
My code follows the style guidelines of this project (formatted with Ruff)
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation if needed
My changes generate no new warnings
I have tested this change
Any dependent changes have been merged and published in downstream modules
I have added all appropriate labels to this PR
I have followed all of these guidelines.
How Has This Been Tested? (if applicable)
I started Tux and ran a few commands.
Screenshots (if applicable)
Additional Information
Please add any other information that is important to this PR.
Summary by Sourcery
Refactor sentry span tagging and command handlers to reduce redundant calls and simplify control flows
Enhancements: