fix(naming): don't lose a series position that isn't a plain number#763
Open
m4bard wants to merge 1 commit into
Open
fix(naming): don't lose a series position that isn't a plain number#763m4bard wants to merge 1 commit into
m4bard wants to merge 1 commit into
Conversation
Audnexus reports a series position as a string, and it is not always a number. An omnibus sits at "1-4" (The Father Brown Collection: Books 1-4), a bundled edition at "1-2" (The Thirty-Nine Steps, which also ships Greenmantle), a prequel at "0" (She and Allan), a novella at "1.5". Two problems followed from squeezing that string through a decimal. Culture. decimal.TryParse was called without a CultureInfo, so it used the server's culture, while FileNamingService formatted the result back with InvariantCulture. The source value always uses '.' as the decimal separator, so under a culture where '.' is the group separator a position of "1.5" parsed as 15, and "0.5" as 5; under fr-FR it did not parse at all. DownloadImportService was worse: it formatted with a bare ToString(), which would write "1,5" -- with a comma -- into a filename. Both parses are now pinned to InvariantCulture and both naming sites format invariantly. Lost positions. A position that failed to parse became null, which is indistinguishable from a book that has no series position. Naming then fell through to the track number and wrote it into the filename as if it were the series number -- so a book at position "1-4" was silently renamed using an unrelated number, with no error and nothing in the log. AudioMetadata now carries SeriesPositionRaw, the position exactly as the source gave it, and naming prefers it over the parsed decimal. The decimal is kept for callers that need to sort or compare. The existing fall back to the track number is deliberate and is preserved for books that genuinely have no series position; the bug was that a real position was being treated as an absent one. Tests cover the culture handling under en-US, de-DE and fr-FR, that a range position survives to the filename rather than being replaced by the track number, and that a book with no position still falls back as before.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #764.
Audnexus reports a series position as a string, and it is not always a number. Squeezing it through a
decimallost it in two ways, and the loss reached the filename.Real positions from the catalogue (public domain, verifiable against
api.audnex.us):positionB0F84DFZ66"1-4"— one ASIN, four booksB002V1PLZK"1-2"— the product also ships GreenmantleB00CQ5WAXW"0"— a prequel slot"1.5"The parse used the server's culture.
decimal.TryParsewas called with noCultureInfo, whileFileNamingServiceformatted the result back withInvariantCulture. The source always uses.as the decimal separator, so where.is the group separator the number changed:A position that didn't parse was silently discarded. It became
null— indistinguishable from a book with no position — so naming fell through to the track number and wrote that into the filename instead. No error, nothing in the log.Changes
Added
AudioMetadata.SeriesPositionRaw— the series position exactly as the metadata source gave it.SeriesPosition(decimal?) is kept for callers that sort or compare, but it cannot hold"1-4"and no longer pretends to.SeriesPositionReproTests— 10 cases covering culture handling and non-numeric positions.Changed
Audiobook.CreateBasicAudioMetadataandDownloadImportServicenow parse the position withInvariantCulture.FileNamingService.Helpers,DownloadImportService) preferSeriesPositionRaw, then the invariant-formatted decimal, then the track number.DownloadImportServicepreviously formatted with a bare.ToString(), which would write1,5— with a comma — into a filename.Fixed
Testing
SeriesPositionReproTests(10 new cases):en-US,de-DEandfr-FR"1-4"and"1-2"are preserved and reach the filename rather than being replaced by the track number"0"survivesdotnet test: 1199 passing, 0 failing (1189 existing + 10 new). No existing test was changed.Notes
The fall back to the track number is deliberate and is preserved — a book with genuinely no series position still gets it. The bug was that a real position was being treated as an absent one.
Apologies for opening this before the issue; #764 has the write-up. I've read
CONTRIBUTING.mdproperly now.Out of scope but worth flagging while it's in view: a book can hold two series memberships at once. Audnexus gives She and Allan (
B00CQ5WAXW)seriesPrimary= Ayesha #0 andseriesSecondary= Allan Quatermain #7.MetadataConvertersreads both, butAudiobookSeriesMembershipis never constructed outside tests, so neither is persisted. Happy to open that separately if useful.