-
Notifications
You must be signed in to change notification settings - Fork 90
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
Fix division by zero #159
Fix division by zero #159
Conversation
whipper/program/cdparanoia.py
Outdated
@@ -196,7 +196,7 @@ def getTrackQuality(self): | |||
|
|||
# don't go over a 100%; we know cdparanoia reads each frame at least | |||
# twice | |||
return min(frames * 2.0 / reads, 1.0) | |||
return min(frames * 2.0 / reads, 1.0) if reads else 0 |
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.
This doesn't seem very Pythonic to me. Something like
if reads:
return min(…)
else:
return 0
looks better to me. Or maybe even just assigning to a variable:
quality = 0
if reads:
quality = min(frames * 2.0 / reads, 1.0)
return quality
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.
Actually I thought about this and wanted to preserve this beauty of code with only one return path (and the min()). If you ask me the most pythonic is to ask for forgiveness and not permission. So what about a try-expect block that catches the division by zero exception?
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.
BTW, I haven't checked: is it OK to have reads with a value of 0? (is there any underlying bug?)
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.
Most likely my CD drive was somewhat broken because I didn't even managed to rip one single CD with it but regardless of that; whipper should catch it IMHO.
In the long run one could think about throwing actual error messages if the drive operates weird (like here).
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.
If that's the reason, I think it's better to rely on the try ... except solution for that's more expressive in this case (unexpected error).
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.
Please rewrite it using a try ... except
Can this be merged now? |
At least nothing to add from my side |
Merged, thanks. |
I would actually request this be un-merged and reviewed for why there's a division by zero happening before we decide to just eat the error. |
@RecursiveForest this is just a fix for a progress indicator which caused whipper to fully crash. It does not influence the rips… |
reads can be zero sometimes an will then cause a ZeroDivisionError in the changed line.
This can happen after calling:
$ whipper offset find