refactor: centralize placeholder substitution logic into shared/functions.py#1186
refactor: centralize placeholder substitution logic into shared/functions.py#1186Ow0cast wants to merge 1 commit intoallthingslinux:mainfrom
Conversation
Reviewer's GuideCentralizes the placeholder substitution logic into a shared utility function and updates all callers (facts plugin, activity handler, and performance tests) to use the new shared implementation. Class diagram for shared substitute_placeholders utilityclassDiagram
class Tux {
}
class Fact {
+Tux bot
+dict facts_data
+_fetch_fact(fact_type str) tuple
+fact_type_autocomplete(interaction AppCommandInteraction, current str) Coroutine
+fact(ctx Context, fact_type str) Coroutine
}
class ActivityHandler {
+Tux bot
+_create_activity_with_substitution(activity Activity) Activity
+build_activity_list() list
}
class Functions {
+substitute_placeholders(bot Tux, text str) str
}
Fact --> Tux : has
ActivityHandler --> Tux : has
Fact ..> Functions : uses substitute_placeholders
ActivityHandler ..> Functions : uses substitute_placeholders
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary of ChangesHello @Ow0cast, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the codebase by centralizing the placeholder substitution logic into a dedicated shared utility module. This change eliminates duplicate code, enhances maintainability, and promotes better organization by providing a single, consistent function for handling text placeholder replacements across different parts of the application. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
📝 WalkthroughWalkthroughThis change centralizes placeholder substitution logic by introducing a new public utility function Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🧰 Additional context used📓 Path-based instructions (6)**/*.py📄 CodeRabbit inference engine (.cursor/rules/rules.mdc)
Files:
src/tux/**/*.py📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/tests/**/*.py📄 CodeRabbit inference engine (.cursor/rules/rules.mdc)
Files:
tests/**/*.py📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/services/**/*.py📄 CodeRabbit inference engine (.cursor/rules/rules.mdc)
Files:
src/tux/services/**/*.py📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (2)📚 Learning: 2026-01-29T20:04:43.675ZApplied to files:
📚 Learning: 2026-01-29T20:04:43.675ZApplied to files:
🧬 Code graph analysis (3)src/tux/plugins/atl/fact.py (1)
tests/performance/test_string_performance.py (1)
src/tux/services/handlers/activity.py (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (13)
✏️ Tip: You can disable this entire section by setting Important Action Needed: IP Allowlist UpdateIf your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:
Reviews will stop working after February 8, 2026 if the new IP is not added to your allowlist. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Now that
substitute_placeholdersis shared, consider narrowing thecontextlib.suppress(Exception)to the specific exceptions you actually expect (e.g.AttributeError,KeyError) so that genuine bugs in new call sites aren’t silently masked. - Since
substitute_placeholdersis now in a broadly used shared module, you might want to make thebotparameter type more flexible (e.g. a Protocol with just the attributes you use) instead of tying it specifically toTux, which will make reuse easier and avoid import-cycle risks for future callers.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Now that `substitute_placeholders` is shared, consider narrowing the `contextlib.suppress(Exception)` to the specific exceptions you actually expect (e.g. `AttributeError`, `KeyError`) so that genuine bugs in new call sites aren’t silently masked.
- Since `substitute_placeholders` is now in a broadly used shared module, you might want to make the `bot` parameter type more flexible (e.g. a Protocol with just the attributes you use) instead of tying it specifically to `Tux`, which will make reuse easier and avoid import-cycle risks for future callers.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
The refactoring to centralize the substitute_placeholders function into shared/functions.py is implemented correctly. All imports have been updated appropriately, function calls have been migrated successfully, and the TYPE_CHECKING pattern is used properly to avoid circular imports. The changes maintain the existing functionality while improving code reusability.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully centralizes the substitute_placeholders function into shared/functions.py, removing duplicated code from plugins/atl/fact.py and services/handlers/activity.py. This is a great refactoring for maintainability. My feedback includes a suggestion to improve the error handling in the new shared function to make it more robust and easier to debug.
| with contextlib.suppress(Exception): | ||
| # Build placeholder map only for placeholders present in text | ||
| placeholders: dict[str, str] = {} | ||
|
|
||
| if "{member_count}" in text: | ||
| placeholders["member_count"] = str( | ||
| sum(guild.member_count or 0 for guild in bot.guilds), | ||
| ) | ||
| if "{guild_count}" in text: | ||
| placeholders["guild_count"] = str(len(bot.guilds) if bot.guilds else 0) | ||
| if "{bot_name}" in text: | ||
| placeholders["bot_name"] = CONFIG.BOT_INFO.BOT_NAME | ||
| if "{bot_version}" in text: | ||
| placeholders["bot_version"] = get_version() | ||
| if "{prefix}" in text: | ||
| placeholders["prefix"] = CONFIG.get_prefix() | ||
|
|
||
| # Single-pass substitution using format_map | ||
| if placeholders: | ||
| text = text.format_map(placeholders) | ||
| return text |
There was a problem hiding this comment.
Using contextlib.suppress(Exception) can hide important errors and make debugging difficult. It's better to catch exceptions and log them. This change will maintain the current behavior of returning the original text on failure but adds valuable logging for troubleshooting.
With this change, you can also remove the import contextlib and add from loguru import logger at the top of the file.
try:
# Build placeholder map only for placeholders present in text
placeholders: dict[str, str] = {}
if "{member_count}" in text:
placeholders["member_count"] = str(
sum(guild.member_count or 0 for guild in bot.guilds),
)
if "{guild_count}" in text:
placeholders["guild_count"] = str(len(bot.guilds) if bot.guilds else 0)
if "{bot_name}" in text:
placeholders["bot_name"] = CONFIG.BOT_INFO.BOT_NAME
if "{bot_version}" in text:
placeholders["bot_version"] = get_version()
if "{prefix}" in text:
placeholders["prefix"] = CONFIG.get_prefix()
# Single-pass substitution using format_map
if placeholders:
text = text.format_map(placeholders)
except Exception as e:
logger.warning(f"Failed to substitute placeholders in '{text}': {e}")
return text
❌ 2 Tests Failed:
View the full list of 2 ❄️ flaky test(s)
To view more test analytics, go to the [Prevent Tests Dashboard](https://All Things Linux.sentry.io/prevent/tests/?preventPeriod=30d&integratedOrgName=allthingslinux&repository=tux&branch=Ow0cast%2Ftux%3Amain) |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 4 files
Confidence score: 3/5
format_map()insrc/tux/shared/functions.pycan throwKeyErroron unknown placeholders and the broad suppression means substitutions may silently fail, which could surface as missing or incorrect text at runtime.contextlib.suppress(Exception)insrc/tux/shared/functions.pyviolates the project’s error-handling guidelines and hides unexpected failures, increasing debugging risk.- Pay close attention to
src/tux/shared/functions.py- silent exception suppression and placeholder handling could mask real errors.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/tux/shared/functions.py">
<violation number="1" location="src/tux/shared/functions.py:470">
P2: Rule violated: **Code Style and Architecture Guidelines**
Using `contextlib.suppress(Exception)` catches all exceptions silently without logging, violating the project's error handling guidelines. Per AGENTS.md, errors should be logged with context.
Consider either:
1. Catching specific expected exceptions (e.g., `KeyError` from `format_map`)
2. Adding logging before suppression to aid debugging
3. Using a more targeted approach that doesn't silently swallow programming errors</violation>
<violation number="2" location="src/tux/shared/functions.py:489">
P2: `format_map()` will raise `KeyError` on any unknown placeholder in `text` (e.g., `{user}`), causing the entire substitution to silently fail due to the broad exception suppression. Consider using a `dict` subclass with `__missing__` to preserve unknown placeholders instead of silently returning unchanged text.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| # Single-pass substitution using format_map | ||
| if placeholders: | ||
| text = text.format_map(placeholders) |
There was a problem hiding this comment.
P2: format_map() will raise KeyError on any unknown placeholder in text (e.g., {user}), causing the entire substitution to silently fail due to the broad exception suppression. Consider using a dict subclass with __missing__ to preserve unknown placeholders instead of silently returning unchanged text.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/tux/shared/functions.py, line 489:
<comment>`format_map()` will raise `KeyError` on any unknown placeholder in `text` (e.g., `{user}`), causing the entire substitution to silently fail due to the broad exception suppression. Consider using a `dict` subclass with `__missing__` to preserve unknown placeholders instead of silently returning unchanged text.</comment>
<file context>
@@ -432,3 +440,51 @@ def dec(obj: Any) -> Any:
+
+ # Single-pass substitution using format_map
+ if placeholders:
+ text = text.format_map(placeholders)
+ return text
</file context>
| if not text: | ||
| return text | ||
|
|
||
| with contextlib.suppress(Exception): |
There was a problem hiding this comment.
P2: Rule violated: Code Style and Architecture Guidelines
Using contextlib.suppress(Exception) catches all exceptions silently without logging, violating the project's error handling guidelines. Per AGENTS.md, errors should be logged with context.
Consider either:
- Catching specific expected exceptions (e.g.,
KeyErrorfromformat_map) - Adding logging before suppression to aid debugging
- Using a more targeted approach that doesn't silently swallow programming errors
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/tux/shared/functions.py, line 470:
<comment>Using `contextlib.suppress(Exception)` catches all exceptions silently without logging, violating the project's error handling guidelines. Per AGENTS.md, errors should be logged with context.
Consider either:
1. Catching specific expected exceptions (e.g., `KeyError` from `format_map`)
2. Adding logging before suppression to aid debugging
3. Using a more targeted approach that doesn't silently swallow programming errors</comment>
<file context>
@@ -432,3 +440,51 @@ def dec(obj: Any) -> Any:
+ if not text:
+ return text
+
+ with contextlib.suppress(Exception):
+ # Build placeholder map only for placeholders present in text
+ placeholders: dict[str, str] = {}
</file context>
1 participant
Pull Request
Description
pretty much a copy paste move of substitute_placeholders from plugins/atl/fact.py to shared/functions.py
If your PR is related to an issue, please include the issue number below:
Related Issue: N/A
Type of Change:
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)
all modified tests pass
Screenshots (if applicable)
Please add screenshots to help explain your changes.
Additional Information
Please add any other information that is important to this PR.