Conversation
|
Caution Review failedThe pull request is closed. Summary by CodeRabbit
WalkthroughThe Azure TTS provider API was simplified: Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
epub2speech/tts/azure_provider.py(1 hunks)epub2speech/tts/protocol.py(1 hunks)tests/test_tts.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_tts.py (4)
tests/utils/config.py (3)
TTSConfig(9-43)validate_config(32-43)get_azure_config(29-30)tests/test_chapter_tts.py (2)
validate_config(123-124)convert_text_to_audio(114-118)epub2speech/tts/azure_provider.py (2)
AzureTextToSpeech(10-71)convert_text_to_audio(36-71)epub2speech/tts/protocol.py (1)
convert_text_to_audio(7-13)
🪛 Ruff (0.13.1)
epub2speech/tts/azure_provider.py
43-43: Avoid specifying long messages outside the exception class
(TRY003)
46-46: Avoid specifying long messages outside the exception class
(TRY003)
62-62: Avoid specifying long messages outside the exception class
(TRY003)
64-64: Avoid specifying long messages outside the exception class
(TRY003)
68-68: Avoid specifying long messages outside the exception class
(TRY003)
71-71: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (1)
epub2speech/tts/protocol.py (1)
11-12: Explicit voice requirement matches current engine needs.Line 11 ensures every caller passes a concrete voice, which lines up with the updated Azure implementation and prevents silent fallbacks. No further action needed here.
| if not text or not text.strip(): | ||
| raise ValueError("Empty text provided for conversion") | ||
|
|
||
| output_path.parent.mkdir(parents=True, exist_ok=True) | ||
|
|
||
| voice = voice or self.default_voice | ||
|
|
||
| self._speech_config.speech_synthesis_voice_name = voice | ||
| audio_config = speechsdk.audio.AudioOutputConfig(filename=str(output_path)) |
There was a problem hiding this comment.
Guard against missing/blank voice input before invoking Azure
With the default voice removed, upstream callers that still rely on configs (or env vars) may hand us None/blank strings. Passing that straight into speech_synthesis_voice_name now blows up inside the SDK with an opaque error, whereas previously we silently fell back to the default voice. Let's fail fast with a clear ValueError and strip accidental whitespace before assigning.
if not text or not text.strip():
raise ValueError("Empty text provided for conversion")
+ if not voice or not voice.strip():
+ raise ValueError("Voice is required and cannot be empty")
+ voice = voice.strip()
+
self._speech_config.speech_synthesis_voice_name = voice📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if not text or not text.strip(): | |
| raise ValueError("Empty text provided for conversion") | |
| output_path.parent.mkdir(parents=True, exist_ok=True) | |
| voice = voice or self.default_voice | |
| self._speech_config.speech_synthesis_voice_name = voice | |
| audio_config = speechsdk.audio.AudioOutputConfig(filename=str(output_path)) | |
| if not text or not text.strip(): | |
| raise ValueError("Empty text provided for conversion") | |
| if not voice or not voice.strip(): | |
| raise ValueError("Voice is required and cannot be empty") | |
| voice = voice.strip() | |
| self._speech_config.speech_synthesis_voice_name = voice | |
| audio_config = speechsdk.audio.AudioOutputConfig(filename=str(output_path)) |
🧰 Tools
🪛 Ruff (0.13.1)
46-46: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In epub2speech/tts/azure_provider.py around lines 45 to 49, add a guard that
checks the provided voice before assigning it to
self._speech_config.speech_synthesis_voice_name: if voice is None or not
str(voice).strip(), raise a ValueError with a clear message (e.g., "No voice
provided for Azure TTS"), otherwise strip() the voice string and then assign the
cleaned value; ensure this check happens before any call into the Azure SDK so
we fail fast with a clear error.
| def setUp(self): | ||
| """Set up test configuration""" | ||
| self.config_path = Path(__file__).parent / "tts_config.json" | ||
| if not self.config_path.exists(): | ||
| self.skipTest("TTS config file not found") | ||
|
|
||
| config = TTSConfig(config_path) | ||
| self.assertTrue(config.validate_config(), "TTS configuration is invalid") | ||
| print("✅ TTS configuration validated") | ||
| config = TTSConfig(self.config_path) | ||
| self.assertTrue(config.validate_config()) | ||
|
|
||
| config = TTSConfig(config_path) | ||
| azure_config = config.get_azure_config() | ||
| self.assertIsNotNone(azure_config, "No Azure configuration found in config file") | ||
|
|
||
| tts = AzureTextToSpeech( | ||
| self.tts = AzureTextToSpeech( | ||
| subscription_key=azure_config["subscription_key"], | ||
| region=azure_config["region"], | ||
| region=azure_config["region"] | ||
| ) | ||
| self.assertTrue(tts.validate_config(), "Azure TTS validation failed") | ||
| print("✅ Azure TTS instance created successfully") | ||
|
|
||
| output_dir = Path(__file__).parent / "dist" | ||
| output_dir.mkdir(exist_ok=True) | ||
| self.output_dir = Path(__file__).parent / "dist" | ||
| self.output_dir.mkdir(exist_ok=True) | ||
|
|
||
| for old_file in output_dir.glob("*.wav"): | ||
| for old_file in self.output_dir.glob("*.wav"): | ||
| old_file.unlink() |
There was a problem hiding this comment.
Prevent cleanup from deleting shared WAV fixtures.
self.output_dir points to <repo>/tests/dist, which is also used by other test modules. Clearing *.wav inside setUp erases audio artifacts those tests rely on, causing racey failures when suites run together.
Tighten the cleanup scope to this test’s own files by either using a test‑specific subdirectory or filtering on a unique prefix:
- self.output_dir = Path(__file__).parent / "dist"
- self.output_dir.mkdir(exist_ok=True)
-
- for old_file in self.output_dir.glob("*.wav"):
- old_file.unlink()
+ self.output_dir = Path(__file__).parent / "dist" / "voice_param"
+ self.output_dir.mkdir(parents=True, exist_ok=True)
+
+ for old_file in self.output_dir.glob("voice_param_*.wav"):
+ old_file.unlink()🤖 Prompt for AI Agents
In tests/test_tts.py around lines 16-35, setUp currently clears all WAVs in
tests/dist which removes shared fixtures used by other tests; change to use a
test-specific output directory (e.g. tests/dist/tts_test) or at minimum only
remove files created by this test by using a unique filename prefix (e.g. tts_*)
— update self.output_dir to a subdirectory, mkdir it if missing, and restrict
the cleanup loop to unlink only files matching that prefix or inside that
subdirectory so other tests' WAV files are not touched.
1 participant
No description provided.