Fix extractor and gating tests#47
Conversation
Reviewer's GuideExtends KeywordExtractor with an audio-based subclass using Whisper (with ImportError guards), simplifies the generate_segments docstring, updates the extractor factory and exports, enforces missing dependency errors in gating, refreshes the README CLI example, and adds a test for the Whisper import dependency. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey @LemurPwned - I've reviewed your changes - here's some feedback:
- Rather than checking for
open_clipandtorchin globals(), wrap their imports in a try/except to more reliably detect missing dependencies. - Consider refactoring
create_extractorto use a dict mapping types to classes instead of chained if-statements, making it easier to add new extractors in the future.
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟡 Testing: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| for seg in result.get("segments", []): | ||
| subtitle_list.append( | ||
| ((int(seg["start"] * 1000), int(seg["end"] * 1000)), seg["text"]) | ||
| ) |
There was a problem hiding this comment.
suggestion: Trim whitespace from Whisper segment text
Call .strip() on seg['text'] to remove extra whitespace and prevent matching issues.
| for seg in result.get("segments", []): | |
| subtitle_list.append( | |
| ((int(seg["start"] * 1000), int(seg["end"] * 1000)), seg["text"]) | |
| ) | |
| for seg in result.get("segments", []): | |
| subtitle_list.append( | |
| ((int(seg["start"] * 1000), int(seg["end"] * 1000)), seg["text"].strip()) | |
| ) |
There was a problem hiding this comment.
do not change the indents
There was a problem hiding this comment.
Good point—let’s keep the existing indent structure and just call .strip() on the text. For example, if your current code looks like this:
for seg in result.get("segments", []):
subtitle_list.append(
((int(seg["start"] * 1000), int(seg["end"] * 1000)),
seg["text"])
)
you can change it to:
for seg in result.get("segments", []):
subtitle_list.append(
((int(seg["start"] * 1000), int(seg["end"] * 1000)),
seg["text"].strip())
)
This way you only trim the whitespace and don’t alter the indentation levels. Let me know if that aligns with your style rules!
| subtitle_list: list[tuple[tuple[int, int], str]] = [] | ||
| for seg in result.get("segments", []): | ||
| subtitle_list.append( | ||
| ((int(seg["start"] * 1000), int(seg["end"] * 1000)), seg["text"]) | ||
| ) |
There was a problem hiding this comment.
suggestion (code-quality): Convert for loop into list comprehension (list-comprehension)
| subtitle_list: list[tuple[tuple[int, int], str]] = [] | |
| for seg in result.get("segments", []): | |
| subtitle_list.append( | |
| ((int(seg["start"] * 1000), int(seg["end"] * 1000)), seg["text"]) | |
| ) | |
| subtitle_list: list[tuple[tuple[int, int], str]] = [ | |
| ((int(seg["start"] * 1000), int(seg["end"] * 1000)), seg["text"]) | |
| for seg in result.get("segments", []) | |
| ] |
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Summary
generate_segmentsin KeywordExtractorImportErrormonkeypatch.setitemTesting
black tests/test_audio_extractor.py video_sampler/language/keyword_capture.py video_sampler/gating.pyisort tests/test_audio_extractor.py video_sampler/language/keyword_capture.py video_sampler/gating.pySummary by Sourcery
Introduce audio-powered keyword extraction using Whisper, enforce dependency checks for the Clip gate, and update documentation and tests accordingly.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests: