fix: address code review findings

This commit is contained in:
2026-07-19 19:22:18 +03:00
parent f1839ad9e7
commit 401c1218b6
4 changed files with 21 additions and 14 deletions

View File

@@ -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

View File

@@ -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

View File

@@ -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 {

View File

@@ -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