fix: default empty MiniMax timber weights#8120
Conversation
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The default timber weight is duplicated as a raw JSON string in both the production code and tests; consider defining it once (e.g. as a Python list/dict constant that you
json.dumpswhere needed) and reusing it to avoid drift and make changes safer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The default timber weight is duplicated as a raw JSON string in both the production code and tests; consider defining it once (e.g. as a Python list/dict constant that you `json.dumps` where needed) and reusing it to avoid drift and make changes safer.
## Individual Comments
### Comment 1
<location path="astrbot/core/provider/sources/minimax_tts_api_source.py" line_range="42-45" />
<code_context>
- '[{"voice_id": "Chinese (Mandarin)_Warm_Girl", "weight": 1}]',
- ),
- )
+ timber_weight = provider_config.get("minimax-timber-weight") or DEFAULT_TIMBER_WEIGHT
+ if isinstance(timber_weight, str) and not timber_weight.strip():
+ timber_weight = DEFAULT_TIMBER_WEIGHT
+ self.timber_weight: list[dict[str, str | int]] = json.loads(timber_weight)
self.voice_setting: dict = {
</code_context>
<issue_to_address>
**issue (bug_risk):** json.loads may now receive non-string values from provider_config, causing runtime errors.
With this change, `provider_config.get("minimax-timber-weight")` may now return an already-parsed Python object instead of a JSON string. In that case `json.loads(timber_weight)` will raise `TypeError`. Consider either converting non-strings to a JSON string before calling `json.loads`, or skipping `json.loads` when the value is already of the expected Python type.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Code Review
This pull request introduces a default value for the minimax-timber-weight configuration and ensures that empty or whitespace-only values correctly fall back to this default. Accompanying unit tests were added to verify this behavior. Feedback suggests defining the default value as a Python list rather than a JSON string to avoid redundant parsing and refactoring the fallback logic to be more robust and idiomatic.
| from ..provider import TTSProvider | ||
| from ..register import register_provider_adapter | ||
|
|
||
| DEFAULT_TIMBER_WEIGHT = '[{"voice_id": "Chinese (Mandarin)_Warm_Girl", "weight": 1}]' |
There was a problem hiding this comment.
Defining the default value as a Python object (list) instead of a JSON string avoids redundant parsing every time a provider instance is created. This is a more idiomatic approach for default configuration values.
| DEFAULT_TIMBER_WEIGHT = '[{"voice_id": "Chinese (Mandarin)_Warm_Girl", "weight": 1}]' | |
| DEFAULT_TIMBER_WEIGHT = [{"voice_id": "Chinese (Mandarin)_Warm_Girl", "weight": 1}] |
| timber_weight = provider_config.get("minimax-timber-weight") or DEFAULT_TIMBER_WEIGHT | ||
| if isinstance(timber_weight, str) and not timber_weight.strip(): | ||
| timber_weight = DEFAULT_TIMBER_WEIGHT | ||
| self.timber_weight: list[dict[str, str | int]] = json.loads(timber_weight) |
There was a problem hiding this comment.
The logic for handling the default value can be simplified and made more robust. The current implementation uses a mix of or and explicit strip() checks that are partially redundant. Additionally, it might fail with a TypeError if the config value is not a string (e.g., if it was already parsed into a list by another part of the system).
By checking if the input is a non-empty string before parsing, we can safely fall back to the default list (assuming DEFAULT_TIMBER_WEIGHT is updated to a list as suggested in the other comment).
| timber_weight = provider_config.get("minimax-timber-weight") or DEFAULT_TIMBER_WEIGHT | |
| if isinstance(timber_weight, str) and not timber_weight.strip(): | |
| timber_weight = DEFAULT_TIMBER_WEIGHT | |
| self.timber_weight: list[dict[str, str | int]] = json.loads(timber_weight) | |
| raw_val = provider_config.get("minimax-timber-weight") | |
| if isinstance(raw_val, str) and raw_val.strip(): | |
| self.timber_weight: list[dict[str, str | int]] = json.loads(raw_val) | |
| else: | |
| self.timber_weight: list[dict[str, str | int]] = DEFAULT_TIMBER_WEIGHT |
55f4e81 to
2d62d89
Compare
Summary
Fixes #8100.
To verify
python -m py_compile astrbot\core\provider\sources\minimax_tts_api_source.py tests\test_minimax_tts_api_source.pypython -m pytest tests\test_minimax_tts_api_source.py -qpython -m ruff check astrbot\core\provider\sources\minimax_tts_api_source.py tests\test_minimax_tts_api_source.pygit diff --checkSummary by Sourcery
Handle MiniMax TTS timber weight configuration defaults more robustly when the saved value is empty or whitespace-only.
Bug Fixes:
Tests: