-
-
Notifications
You must be signed in to change notification settings - Fork 39
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
VorbisComments: Support current/total TRACKNUMBER fields #500
Conversation
6ea135a
to
3040351
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this! Just one suggestion.
value_split.next().and_then(|b| b.parse().ok()); | ||
let track_total: Option<u32> = | ||
value_split.next().and_then(|b| b.parse().ok()); | ||
if let Some(n) = track_number { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The workaround here also mentions vinyl track numbers. I think in the case that the track number doesn't parse, just insert it into the tag like any other item.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added vinyl track numbers and another test. Also pushing this tag if track number can't be parsed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the confusion! I meant Lofty shouldn't do anything special with vinyl tracks, since I imagine there are weird formats for those out in the wild. In the case that no track number is parsed, just insert it into the tag like normal.
Made that change in e961455
(#500)
12a16c5
to
4a18f79
Compare
Support <current>/<total> and vinyl track number (such as A2, b5) in TRACKNUMBER field Closes: Serial-ATA#499
For these kind of unit tests it is not required to add more test files. The tags to be parsed could be constructed programmatically during the tests and we already use this approach in many cases. Using actual test files should be reserverd for testing the (binary) deserialization, not the semantic parsing. |
@uklotzde I couldn't immediately figure out how to do that. I will try to change tests later. Could you point to test where tags are constructed programmatically? |
Closes: #499