-
Notifications
You must be signed in to change notification settings - Fork 30
fix: enhanced LRC parsing #289
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
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideHandles failures in enhanced LRC word-level time tag parsing by falling back to a synthetic time tag instead of failing the whole line, ensuring non-enhanced lyrics with stray '<' characters are still parsed. 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 there - I've reviewed your changes - here's some feedback:
- Using
unwrap_or_elseto swallow allparse_enhanced_lrcerrors and silently fabricating aTimeTagwithminutes: 9999introduces a magic value and hides parsing failures; consider either returning a distinct variant/flag for the fallback case or funneling this through a dedicated helper with a clearly named constant to make the behavior explicit. - The fallback branch currently allocates a
Vecwith a single tuple and clones/allocatesstart_timeandtextsimilarly to the non-enhanced case; consider refactoring the shared tuple construction into a small helper to avoid duplication and make the intent of the two branches clearer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Using `unwrap_or_else` to swallow all `parse_enhanced_lrc` errors and silently fabricating a `TimeTag` with `minutes: 9999` introduces a magic value and hides parsing failures; consider either returning a distinct variant/flag for the fallback case or funneling this through a dedicated helper with a clearly named constant to make the behavior explicit.
- The fallback branch currently allocates a `Vec` with a single tuple and clones/allocates `start_time` and `text` similarly to the non-enhanced case; consider refactoring the shared tuple construction into a small helper to avoid duplication and make the intent of the two branches clearer.
## Individual Comments
### Comment 1
<location> `lyric/src/lrc.rs:69-70` </location>
<code_context>
+ parse_enhanced_lrc(remaining_content).unwrap_or_else(|_| {
+ vec![(
+ start_time.clone(),
+ TimeTag {
+ minutes: 9999,
+ seconds: 0,
+ milliseconds: 0,
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Using `minutes: 9999` as a sentinel end time is a fragile magic constant and may break ordering or duration assumptions.
Using an artificial `9999`-minute end time can violate assumptions about valid track duration and makes the intent unclear. Prefer a named constant (e.g., `FALLBACK_END_TIME`) or a more explicit model like `Option<TimeTag>` for “no end time,” ideally consistent with how the non-enhanced path handles missing word-level tags.
Suggested implementation:
```rust
/// Fallback end time used when enhanced LRC parsing fails and no explicit
/// word-level end time is available. This is intentionally large, but
/// should be treated as a sentinel and not as a real track duration.
const FALLBACK_END_TIME: TimeTag = TimeTag {
minutes: 9999,
seconds: 0,
milliseconds: 0,
};
// Parse enhanced format word-level time tags
let word_time_tags = if remaining_content.contains('<') {
```
```rust
parse_enhanced_lrc(remaining_content).unwrap_or_else(|_| {
vec![(
start_time.clone(),
FALLBACK_END_TIME,
text.to_string(),
)]
})
```
If `TimeTag` or this parsing logic is reused elsewhere, you may also want to:
1. Move `FALLBACK_END_TIME` to a more central location (e.g., near the `TimeTag` definition) and make it `pub const` if needed by other modules.
2. Add a unit test that verifies the fallback behavior when `parse_enhanced_lrc` returns an error, asserting that `FALLBACK_END_TIME` is used instead of an inline magic value.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This pull request refactors how dummy end times are handled in the lyric parsing logic and improves the robustness of milliseconds parsing in time tags. The main changes include introducing a reusable constant for dummy end times, updating all relevant code to use this constant, and enhancing the parsing of milliseconds to support both two- and three-digit formats.
Lyric parsing improvements
DUMMY_END_TIMEconstant inlrc.rsto standardize the representation of placeholder end times throughout the file.parse_lrcandparse_enhanced_lrcto use the newDUMMY_END_TIMEconstant, improving code clarity and maintainability. [1] [2]Time tag parsing enhancements
FromStrimplementation forTimeTagintypes.rsto support parsing milliseconds with both two and three digits, and to handle invalid formats more gracefully.