-
-
Notifications
You must be signed in to change notification settings - Fork 40
refactor(tempvc): use list comprehensions, add TEMPVC_BASE_NAME #951
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
base: main
Are you sure you want to change the base?
Conversation
Reviewer's GuideThis PR adds a new configuration variable TEMPVC_BASE_NAME, integrates it into the config loader, updates the TempVc service to use this variable with a fallback, and updates the example settings file accordingly. Class diagram for updated TempVc and Config classesclassDiagram
class TempVc {
- bot: Tux
- base_vc_name: str
+ __init__(bot: Tux)
}
class Config {
+ TEMPVC_CATEGORY_ID: str | None
+ TEMPVC_CHANNEL_ID: str | None
+ TEMPVC_BASE_NAME: str | None
+ RECENT_GIF_AGE: int
}
TempVc --> Config : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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 @amadaluzia - I've reviewed your changes - here's some feedback:
- Use an explicit None check (e.g. CONFIG.TEMPVC_BASE_NAME if CONFIG.TEMPVC_BASE_NAME is not None else '/tmp/') instead of
or
so that empty strings are honored correctly. - Normalize the provided base VC name (e.g. trim or enforce a trailing separator) to prevent inconsistent channel names when admins supply custom values.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Use an explicit None check (e.g. CONFIG.TEMPVC_BASE_NAME if CONFIG.TEMPVC_BASE_NAME is not None else '/tmp/') instead of `or` so that empty strings are honored correctly.
- Normalize the provided base VC name (e.g. trim or enforce a trailing separator) to prevent inconsistent channel names when admins supply custom values.
## Individual Comments
### Comment 1
<location> `tux/cogs/services/temp_vc.py:11` </location>
<code_context>
def __init__(self, bot: Tux) -> None:
self.bot = bot
- self.base_vc_name: str = "/tmp/"
+ self.base_vc_name: str = CONFIG.TEMPVC_BASE_NAME or "/tmp/"
@commands.Cog.listener()
</code_context>
<issue_to_address>
Potential issue if TEMPVC_BASE_NAME is set to an empty string.
Empty strings are falsy in Python, so the fallback to "/tmp/" will trigger if TEMPVC_BASE_NAME is set to "". If you only want the fallback when TEMPVC_BASE_NAME is None, use 'CONFIG.TEMPVC_BASE_NAME if CONFIG.TEMPVC_BASE_NAME is not None else "/tmp/"'.
</issue_to_address>
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
d9ab393
to
38616f2
Compare
Please resolve the failed test at https://github.com/allthingslinux/tux/actions/runs/16307237637/job/46220855246?pr=951 |
Description
TEMPVC_BASE_NAME
This adds a new config variable called TEMPVC_BASE_NAME which will allow an admin to change the default temporary VC channel base name. This will allow for better customisabillity in Tux which I see as an end goal of the project. I have also added it to the example.
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 joined a channel after changing the value and it had the exact value I expected.
Screenshots (if applicable)
TEMPVC_BASE_NAME: "tmp: "
TEMPVC_BASE_NAME: "new value "
Summary by Sourcery
Enable customization of temporary voice channel base names by adding and using a new TEMPVC_BASE_NAME configuration option
New Features:
Enhancements:
Documentation: