-
-
Notifications
You must be signed in to change notification settings - Fork 34
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
APE: Properly convert track/disk number pairs when writing #216
Conversation
This also changes the default track/disk number to `DEFAULT_NUMBER_IN_PAIR`. closes #159
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.
Parsing of numbers needs to be more robust and more liberal. The special cases should be tested.
@@ -18,7 +18,7 @@ byteorder = "1.4.3" | |||
# ID3 compressed frames | |||
flate2 = { version = "1.0.26", optional = true } | |||
# Proc macros | |||
lofty_attr = "0.8.0" | |||
lofty_attr = { path = "lofty_attr" } |
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.
Intentional change?
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.
Yes, it used ape::tag::tagitems_into_ape
, which was changed.
src/id3/v2/util/pairs.rs
Outdated
/// Attempts to convert a `TagItem` to a number, passing it to `setter` | ||
pub(crate) fn set_number<F: FnMut(u32)>(item: &TagItem, mut setter: F) { | ||
let text = item.value().text(); | ||
let number = text.map(str::parse::<u32>); |
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.
What happens if a field is empty or solely contains whitespace characters? These cases should probably be interpreted like a missing field.
The parsing should also not fail if the number is enclosed in leading/trailing whitespace characters, e.g. parsing " 12 "
should not fail.
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 text should be trimmed and then checked for emptiness before trying to parse the number.
Now whitespace will be trimmed and empty strings will be ignored.
src/id3/v2/util/pairs.rs
Outdated
|
||
/// Attempts to convert a `TagItem` to a number, passing it to `setter` | ||
pub(crate) fn set_number<F: FnMut(u32)>(item: &TagItem, mut setter: F) { | ||
let text = item.value().text().map(str::trim); |
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.
Too many local variables with confusing naming. How about this:
let trimmed_text = item.value().text().unwrap_or_default().trim();
if trimmed_text.is_empty() {
...early return...
}
This also changes the default track/disk number to
DEFAULT_NUMBER_IN_PAIR
.closes #159