ID3v2: Ignore frames with bad IDs while reading

[uklotzde]

Follow-up of #212.

[uklotzde]

CI fails again due to unrelated rustfmt errors.

[Serial-ATA]

Ok(None) is currently used to indicate that we encountered padding or the reader is exhausted. This won't only ignore the bad frame ID, but stop reading the tag entirely.

I think errors should continue to be thrown from parse_*_header, and handled once in Frame::read. That'd simplify things a bit, since the match on parse_mode appears 3 times in this file.

From there, you can return Ok((None, false)) from Frame::read to indicate that there are more frames to come after the failure:

lofty-rs/src/id3/v2/read.rs

Lines 50 to 56 in f1e8ab6

	match Frame::read(reader, header.version, parse_mode)? {
	// No frame content found, and we can expect there are no more frames
	(None, true) => break,
	(Some(f), false) => drop(tag.insert(f)),
	// No frame content found, but we can expect more frames
	_ => {},
	}

[uklotzde]

Then Option is the wrong choice here and very confusing. It needs to be replaced by a dedicated type that reveals the semantics.

[Serial-ATA]

Definitely. This isn't the only place where Options and bools are being misused. It's something I'll have to look into eventually.

[Serial-ATA]

I added a test for this, and just realized that the frame will not actually be skipped over.

There would need to be a way to get the size out of the header even if there's an error.

[uklotzde]

Fixed.

Code could be improved later to avoid the mutable reference param. Not urgent.

[uklotzde]

The naming of the ParsedFrame variant is more appropriate now, because the skipping actually happens later.

[Serial-ATA]

Thanks!

I'll move Clippy/rustfmt to stable, the failures are annoying.