refactor(treewide): reorder statements to improve speed #953
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, avatar command, and xkcd command by caching sentry initialization checks, reordering conditionals, and flattening nested logic for improved performance and readability. Class diagram for CogLoader sentry optimizationclassDiagram
class CogLoader {
+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)
-IS_INITIALIZED: bool
}
class sentry_sdk {
+is_initialized()
+get_current_span()
}
CogLoader --|> sentry_sdk : uses
note for CogLoader "Now uses IS_INITIALIZED instead of repeated sentry_sdk.is_initialized() calls"
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 ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## main #953 +/- ##
========================================
+ Coverage 9.26% 9.39% +0.12%
========================================
Files 123 123
Lines 10390 10388 -2
Branches 1276 1274 -2
========================================
+ Hits 963 976 +13
+ Misses 9325 9298 -27
- Partials 102 114 +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
| @@ -14,6 +14,8 @@ | |||
| from tux.utils.config import CONFIG | |||
| from tux.utils.sentry import safe_set_name, span, start_span, transaction | |||
|
|
|||
| IS_INITIALIZED: bool = sentry_sdk.is_initialized() | |||
There was a problem hiding this comment.
it's not immediately clear what IS_INITIALIZED is referring to exactly. consider naming the variable something like SENTRY_INITIALIZED instead for clarity.
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.
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 performance-critical code by caching the sentry initialization state and reorganizing conditional statements in cog loading and command handlers to reduce redundant checks and simplify logic.
Enhancements: