Skip to content

Conversation

Methapon2001
Copy link
Contributor

@Methapon2001 Methapon2001 commented Aug 18, 2025

Summary

Fixes #540.
This PR make VorbisComments parse TRACKNUMBER with respect to ParseOption::implicit_conversions

Note

Let me know if there is something more that should be done.

I'm not sure if I need to update the Accessor for VorbisComments. The goal is to ensure that track and track_total can still be called and return u32 values, even when the format is <current>/<total>.

	fn track(&self) -> Option<u32> {
		if let Some(item) = self
			.get("TRACKNUMBER")
			.map_or_else(|| self.get("TRACKNUM"), Some)
		{
			return item.split('/').next().and_then(|v| v.parse::<u32>().ok());
		}

		None
	}

What do you think?

@Serial-ATA
Copy link
Owner

Thanks!

The goal is to ensure that track and track_total can still be called and return u32 values, even when the format is <current>/<total>.

Hm, I'm not sure how I feel about Accessor methods handling multiple data formats like that. Generally, I prefer modifying the data at parse time, and having Accessor assume that's the case. That's what implicit_conversions warns about in the docs: https://docs.rs/lofty/latest/lofty/config/struct.ParseOptions.html#method.implicit_conversions

@Methapon2001
Copy link
Contributor Author

Thanks!

The goal is to ensure that track and track_total can still be called and return u32 values, even when the format is <current>/<total>.

Hm, I'm not sure how I feel about Accessor methods handling multiple data formats like that. Generally, I prefer modifying the data at parse time, and having Accessor assume that's the case. That's what implicit_conversions warns about in the docs: https://docs.rs/lofty/latest/lofty/config/struct.ParseOptions.html#method.implicit_conversions

All right, then the current behaviors should be as expected. I also added 2 test cases.

@Serial-ATA
Copy link
Owner

I went ahead and removed the test files, for a simple case like this it's best to just create the tag in the test

@Serial-ATA Serial-ATA merged commit 4f81ef4 into Serial-ATA:main Aug 18, 2025
4 checks passed
@Serial-ATA Serial-ATA added this to the 0.23.0 milestone Aug 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

VorbisComments: TRACKNUMBER=current/total handling should be under implicit conversions
2 participants