Capitalize first letter of generated captions#68
Conversation
📝 WalkthroughWalkthroughA new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
util.py (1)
16-20: ⚡ Quick winAdd a focused test for this helper.
Since this now drives caption casing across multiple emitters, a small regression here would fan out widely. Please cover empty input, leading punctuation/whitespace, and already-capitalized text.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@util.py` around lines 16 - 20, Add a focused unit test suite for the capitalize(text: str) helper in util.py that verifies behavior for empty input, leading punctuation/whitespace, and already-capitalized text: create tests calling capitalize("") expecting "", capitalize(" -hello") expecting the first alphabetic character to be capitalized (" -Hello"), and capitalize("Already") expecting "Already" (unchanged); also include a test for a lowercase word like "word" -> "Word" to ensure basic functionality remains correct and assert exact string equality for each case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@util.py`:
- Around line 16-20: Add a focused unit test suite for the capitalize(text: str)
helper in util.py that verifies behavior for empty input, leading
punctuation/whitespace, and already-capitalized text: create tests calling
capitalize("") expecting "", capitalize(" -hello") expecting the first
alphabetic character to be capitalized (" -Hello"), and capitalize("Already")
expecting "Already" (unchanged); also include a test for a lowercase word like
"word" -> "Word" to ensure basic functionality remains correct and assert exact
string equality for each case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7e7118f9-d257-4544-8859-dad8a7737172
📒 Files selected for processing (6)
extractors/semantic.pyextractors/statistical.pyextractors/structural.pysynthesizers/_helper.pysynthesizers/sleep.pyutil.py
♻️ Current situation & Problem
Several caption templates start with a placeholder (
{name},{activity_name}) that resolves to a lowercase value (e.g., "heart rate", "active energy", "yoga", "running"). Rendered captions therefore start with a lowercase letter — e.g., "heart rate centered at 80 bpm…", "running took up minute 10 to 30.".extractors/semantic.pyalready worked around this with an inline `caption[0].upper() + caption[1:]`, but the other emit sites didn't.⚙️ Release Notes
Metric-suffix fragments built in `_helper.py` (e.g. "averaging a heart rate of 80 bpm") are intentionally left lowercase since they are concatenated mid-sentence after a leading caption.
📚 Documentation
No template changes needed; capitalization is now applied at the rendering boundary, so future templates can keep using lowercase placeholders without producing ungrammatical sentence starts.
✅ Testing
Code of Conduct & Contributing Guidelines
By creating and submitting this pull request, you agree to follow our Code of Conduct and Contributing Guidelines: