whipper icon indicating copy to clipboard operation
whipper copied to clipboard

Reword 'track n: unknown (error)' to reflect missing from AccurateRip DB

Open Bujiraso opened this issue 3 years ago • 4 comments

Hello,

This wording catches me off guard wrong and I continually check for errors but it looks like the track is simply missing from the database.

Could this be re-worded to

"track n: unknown          (track not present in AccurateRip database for verification)"

?

I'm used to "error" meaning I need to do something, but as far as I understand there's nothing I can do here. The extra info would be helpful so I don't need to crack the .log file open each time.

Let me know if this seems something worth a "beginner" PR from me! Thanks!

Bujiraso avatar Mar 14 '21 12:03 Bujiraso

Hi, thanks for the issue report. I agree that the current message may be a bit too terse but keep in mind that the one you provided could be incorrect in case the given track does really exist in the AccurateRip DB but the extracted one has a different CRC (maybe because of read errors).

P.S.: The relevant code section is this one:

https://github.com/whipper-team/whipper/blob/04ff0058064b52ff9262a4a4820cb8b2b0b95125/whipper/common/accurip.py#L237-L239

In case that message gets changed, the associated tests need to be updated too.

JoeLametta avatar Mar 22 '21 12:03 JoeLametta

Thanks for the feedback on this, @JoeLametta.

I was surprised to read there's another option here where the tracks do exist but the CRCs could be wrong! I have a handful of "Unknowns" right now, is there any way to tell if they are in the bad-read vs. absent state?

As you say, it looks like print report function does not distinguish this -- is it correct to rephrase this problem as "whipper doesn't provide any different response to the user for these two given states (found, not matching and entirely absent)"?

If yes, while designing this, a valuable part could be to divide these two error states so that the user is clear as to whether or not the fault could be on AccurateRip's lack of populated data vs. read errors and the like, as they're rather different.

Bujiraso avatar Mar 23 '21 21:03 Bujiraso

As you say, it looks like print report function does not distinguish this -- is it correct to rephrase this problem as "whipper doesn't provide any different response to the user for these two given states (found, not matching and entirely absent)"?

I think that's not correct: whipper should issue the following messages in case no remote CRCs are found for the given track:

https://github.com/whipper-team/whipper/blob/04ff0058064b52ff9262a4a4820cb8b2b0b95125/whipper/common/accurip.py#L237-L239

In case no entry is found for the given CD, there are at least these (second one for the cd subcommand):

https://github.com/whipper-team/whipper/blob/04ff0058064b52ff9262a4a4820cb8b2b0b95125/whipper/common/accurip.py#L141-L151

https://github.com/whipper-team/whipper/blob/87f3d00ee3686fe43f0d3a931dea6361c71a1461/whipper/command/cd.py#L545-L550

These are some of the cases, taken from our tests, which can be reported in relation to the AccurateRip status report:

https://github.com/whipper-team/whipper/blob/87f3d00ee3686fe43f0d3a931dea6361c71a1461/whipper/test/test_common_accurip.py#L190-L279

This is the specific situation you've mentioned in your first message (it means no AccurateRip entry has been found for the given track in the remote DB):

https://github.com/whipper-team/whipper/blob/87f3d00ee3686fe43f0d3a931dea6361c71a1461/whipper/test/test_common_accurip.py#L215-L223

JoeLametta avatar Mar 24 '21 11:03 JoeLametta

That all sounds agreeable and after learning a bit more, I understand your meaning.

As I'm still learning, it is correct to paraphrase that "the track may indeed be in the AccurateRip database under a different disc (ID?) but the CRC given did not identify it"?

This seems to happen to me for entire discs -- could it ever happen on a single track in a disc? If not, then perhaps this is closer: track n: disc unknown (disc did not return AccurateRip entry for CRC verification)

WDYT, @JoeLametta - did I get closer or farther 🙂?

Bujiraso avatar Mar 26 '21 20:03 Bujiraso