-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
12660 progress and small optimization #13244
base: main
Are you sure you want to change the base?
Conversation
Welcome, new contributor! Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨ |
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.
Nice, small PR. Some of the diff is mainly from formatting but I guess there's no way around it.
Co-authored-by: Pascal Hertleif <[email protected]>
Co-authored-by: Pascal Hertleif <[email protected]>
Co-authored-by: Pascal Hertleif <[email protected]>
Co-authored-by: François Mockers <[email protected]>
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.
I think messages in expect
should be more helpful than some of those to be useful. They should provide some more context than reading the code does:
- as the API user, does this helps me with debugging without reading the code?
- does it provides me info on which inputs I gave that created the panic?
- does it informs me on why they are invalid?
Replacing unwrap
by expect
without providing this kind of details is not helpful, and will require people who want to improve it later to understand more context, it includes strings in the binary that takes up a small place, and provides a false sense of "we don't use unwrap"
let search_result = self.keyframe_timestamps.binary_search_by(|probe| { | ||
probe | ||
.partial_cmp(&seek_time) | ||
.expect("provided floats can't be compared") |
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 are those floats? why are they compared? which is an input provided by the user when calling this function?
Objective
small effort towards #12660 along with a small optimization found in the process
first commit