fix(metadata): order series positions with an invariant parse - #892
Open
m4bard wants to merge 2 commits into
Open
fix(metadata): order series positions with an invariant parse#892m4bard wants to merge 2 commits into
m4bard wants to merge 2 commits into
Conversation
An Audible series position arrives as a string that always uses '.' as the decimal separator, so parsing it under the server's culture makes the order depend on the host locale. Where '.' is the group separator a position of "1.5" read as 15 and the novella sorted after book 10; where ',' is the decimal separator it did not parse at all and fell to decimal.MaxValue. A stock container runs under the invariant culture and was unaffected. The bug needed a real culture to reach the process, which happens when LANG or LC_ALL is set and on a desktop install that inherits the OS locale. Non-numeric positions such as an omnibus at "1-4" still sort last, which is unchanged and deliberate. ParseSeriesPosition becomes internal so the culture behaviour can be asserted against the real method rather than a copy of it. Fixes Listenarrs#795
…#717 added This branch predated TestClasses_FollowRepositoryConventions, so the test class inherited nothing and carried neither trait. Inherit BaseTests and add the exact Name trait and a Category, which is what the architecture suite now requires.
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.
Fixes #795.
A series position arrives from Audible's
sequence/sortfield as a string that always uses.as the decimal separator.ParseSeriesPositionparsed it with the ambient culture, so on a server whose culture treats.as the group separator the value was read as a different number.A default container runs under the invariant culture and was never affected, which is why this is easy to miss. It shows up once the process has a real culture, which happens when
LANGorLC_ALLis set and on a desktop install that inherits the operating system's locale.The parse is now pinned to
CultureInfo.InvariantCulturewithNumberStyles.Number. Sort data, parsed the same way everywhere.The tiebreak question from the issue, deliberately not answered here
I asked on the issue what should happen to a position that is not a decimal at all, the
1-4of an omnibus. This PR does not change that: those still sort last, exactly as they do today.That is not me deciding the question. It is me keeping this change to the part that is unambiguously a bug, so the ordering of omnibus entries stays whatever you decide it should be, separately. If you would rather they sorted first, or kept their catalogue order, that is a different change and I am happy to write it once you say which.
Tests
SeriesPositionOrderingTests, thirteen cases. The parse is asserted directly underde-DE,fr-FRand the invariant culture, and the ordering is asserted end to end over a mixed series.Control, with only the parse reverted to the culture-dependent form and everything else left in place:
That last line is the reported symptom: the novella lands after book 10.
ParseSeriesPositiongoes fromprivatetointernalso the culture behaviour can be asserted on the method rather than inferred from the ordering. That accessibility change is why a plain revert of the whole commit will not compile; the control above reverts only the parse.Full suite 3051 passed, 0 failed, 127 skipped, on canary
d25b3e11(1.3.3).One housekeeping commit alongside: this branch predated
TestClasses_FollowRepositoryConventions, so the test class neededBaseTestsand its traits to satisfy the architecture suite.