Skip to content

Conversation

@clover-yan
Copy link
Contributor

@clover-yan clover-yan commented Dec 8, 2025

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

  • Introduced a DUMMY_END_TIME constant in lrc.rs to standardize the representation of placeholder end times throughout the file.
  • Updated all usages of hardcoded dummy end times in parse_lrc and parse_enhanced_lrc to use the new DUMMY_END_TIME constant, improving code clarity and maintainability. [1] [2]

Time tag parsing enhancements

  • Modified the FromStr implementation for TimeTag in types.rs to support parsing milliseconds with both two and three digits, and to handle invalid formats more gracefully.

@sourcery-ai
Copy link

sourcery-ai bot commented Dec 8, 2025

Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

Handles 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

Change Details Files
Add fallback behavior when parsing enhanced LRC word-level time tags fails so that the line still produces a word_time_tag entry.
  • Wrap parse_enhanced_lrc call with unwrap_or_else to catch parsing errors
  • On parse failure, create a single word_time_tag using the line start_time, a sentinel TimeTag value (minutes=9999, seconds=0, milliseconds=0), and the original text
  • Keep the existing non-enhanced path (no '<' present) unchanged
lyric/src/lrc.rs

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link

@sourcery-ai sourcery-ai bot left a 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_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.
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>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@dosubot dosubot bot added the C - bug Something isn't working label Dec 8, 2025
@clover-yan clover-yan changed the title fix: lyrics without enhanced format word-level time tags treated wrongly fix: enhanced LRC parsing Dec 8, 2025
@Losses Losses merged commit a8cc486 into Losses:master Dec 15, 2025
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C - bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants