From 401c1218b6b5f12d16dd171b73fc7c2b91ab8a5c Mon Sep 17 00:00:00 2001 From: Vladimir Zagainov Date: Sun, 19 Jul 2026 19:22:18 +0300 Subject: [PATCH] fix: address code review findings --- internal/musicbrainz/sync.go | 2 +- internal/normalize/normalize.go | 21 ++++++++++++--------- internal/normalize/normalize_test.go | 6 ++++++ internal/scanner/scan.go | 6 ++---- 4 files changed, 21 insertions(+), 14 deletions(-) diff --git a/internal/musicbrainz/sync.go b/internal/musicbrainz/sync.go index b25b969..63f4425 100644 --- a/internal/musicbrainz/sync.go +++ b/internal/musicbrainz/sync.go @@ -57,7 +57,7 @@ func SyncArtistDiscography( if err != nil { return nil, fmt.Errorf("sync artist discography: read artist filter options: %w", err) } - filtered := cachedReleases[:0] + filtered := make([]database.ExternalRelease, 0, len(cachedReleases)) for _, r := range cachedReleases { if opts.IgnoreSingles && r.Type == "Single" { continue diff --git a/internal/normalize/normalize.go b/internal/normalize/normalize.go index 2986ffe..9ae5f24 100644 --- a/internal/normalize/normalize.go +++ b/internal/normalize/normalize.go @@ -18,10 +18,11 @@ var ( parenRe = regexp.MustCompile(`\([^)]*\)`) yearRe = regexp.MustCompile(`\b(1[0-9]{3}|2[0-9]{3})\b`) spaceRe = regexp.MustCompile(`\s+`) - // wordRe matches any alphabetic character. Used to decide whether a title - // that collapses entirely to a year actually had other words worth keeping - // (e.g. "1989 (Deluxe)") versus being a bare year title (e.g. "1989"). - wordRe = regexp.MustCompile(`[a-z]`) + // bareYearRe matches a title that is *only* a single year (with optional + // surrounding whitespace), e.g. "1989" or "2112". Used to decide whether a + // title that collapses entirely to a year should keep it (so it matches + // itself) or be treated as a distinct reissue that must collapse to empty. + bareYearRe = regexp.MustCompile(`^\s*(1[0-9]{3}|2[0-9]{3})\s*$`) ) // NormalizeString normalizes a string for fuzzy matching by: @@ -46,20 +47,22 @@ func NormalizeString(s string) string { s = parenRe.ReplaceAllString(s, "") // Remove years (4-digit numbers between 1000-2999). If stripping the year - // would empty the entire string, we must decide what to keep: + // empties the entire string, decide what to keep: // - A bare year title (e.g. "1989", "2112") has no other words, so keep // the year so it can still match itself (the user owns that album). // - A title that had OTHER words alongside the year (e.g. "1989 (Deluxe)") // collapses to empty on purpose: it is a distinct release group that // must NOT be considered already-present just because the user owns the // standard "1989". Collapsing to empty makes it score 0.0 against a - // plain "1989", correctly reporting the reissue as missing. + // plain "1989", correctly reporting the reissue as missing. The check + // is against the original (brackets intact) so a title like "1989 + // [2020]" is correctly NOT treated as a bare year. stripped := yearRe.ReplaceAllString(s, "") if strings.TrimSpace(stripped) == "" { - if wordRe.MatchString(strings.ToLower(original)) { - s = "" - } else { + if bareYearRe.MatchString(strings.TrimSpace(original)) { s = strings.TrimSpace(s) + } else { + s = "" } } else { s = stripped diff --git a/internal/normalize/normalize_test.go b/internal/normalize/normalize_test.go index 010e382..3876688 100644 --- a/internal/normalize/normalize_test.go +++ b/internal/normalize/normalize_test.go @@ -44,6 +44,12 @@ func TestNormalizeString_Basic(t *testing.T) { {"1989 (Deluxe)", ""}, {"1989 [Deluxe Edition]", ""}, {"2112 (Remastered)", ""}, + // Regression: a year with a bracketed/suffixed year must NOT collapse to + // the bare year (it falsely matched "1989" before). It collapses to empty. + {"1989 [2020]", ""}, + {"1989 2020", ""}, + {"3000 2000", "3000"}, + {"1989 RMX", "rmx"}, } for _, tt := range tests { diff --git a/internal/scanner/scan.go b/internal/scanner/scan.go index 18a874c..f6d15f4 100644 --- a/internal/scanner/scan.go +++ b/internal/scanner/scan.go @@ -28,7 +28,7 @@ func ScanArtist(ctx context.Context, db *database.DB, artistID string, threshold return nil, err } - missing := FindMissingReleases(local, external, resolveThreshold(threshold)) + missing := FindMissingReleases(local, external, threshold) return missing, nil } @@ -46,8 +46,6 @@ func ScanAll(ctx context.Context, db *database.DB, threshold float64) ([]Missing return nil, err } - resolved := resolveThreshold(threshold) - var all []MissingRelease for _, s := range settings { if err := ctx.Err(); err != nil { @@ -58,7 +56,7 @@ func ScanAll(ctx context.Context, db *database.DB, threshold float64) ([]Missing } // A transient error for one artist must not abort the whole scan and // take down the daemon; log it and continue with the remaining artists. - missing, err := ScanArtist(ctx, db, s.ID, resolved) + missing, err := ScanArtist(ctx, db, s.ID, threshold) if err != nil { log.Printf("scan artist %s failed: %v", s.ID, err) continue