-
Notifications
You must be signed in to change notification settings - Fork 272
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
overload RationalTime.from_seconds with additional rate argument #1118
overload RationalTime.from_seconds with additional rate argument #1118
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1118 +/- ##
==========================================
- Coverage 86.20% 86.19% -0.02%
==========================================
Files 191 191
Lines 19028 19049 +21
Branches 2105 2104 -1
==========================================
+ Hits 16404 16420 +16
- Misses 2078 2083 +5
Partials 546 546
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Hi! All tests seem to pass so I'd like some feedback on this PR. What I'm most concerned about is the pybind11 stuff and if I'm introducing memory leaks etc. And please let me know if the PR should go back to draft. The review request seemed to remove the draft status. |
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.
Looks good, but what do you think about a simpler unit test? Or was there something specific you wanted to achieve with the datetime approach?
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.
Closes #818
Summarize your change.
Work in progress!!
I'm in over my head here so hacking together an attempt.
Overload
RationalTime.from_seconds
with an additional "rate" argument to avoid having to rescale value to target rate.Before PR:
After PR:
Reference associated tests.
Tests will be added to the unittests