-
Notifications
You must be signed in to change notification settings - Fork 273
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
Make types in opentime immutable. #372
Make types in opentime immutable. #372
Conversation
@alatdneg you might also be interested in this change. |
Codecov Report
@@ Coverage Diff @@
## master #372 +/- ##
==========================================
- Coverage 87.07% 86.96% -0.12%
==========================================
Files 63 63
Lines 5580 5600 +20
==========================================
+ Hits 4859 4870 +11
- Misses 721 730 +9
Continue to review full report at Codecov.
|
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 really like the shift toward immutability on the opentime types, I think it'll make the type feel a lot more like other number types in python and do a good job guarding against unexpected programming errors (especially the iadd
use case).
In terms of conforming to float
, I think this brings to the front the conversation about data types in RationalTime
. That conversation is a bit larger, so I put an analysis over in the group discussion:
https://groups.google.com/d/msg/open-timeline-io/a57pa3-DxEQ/LTVmEkmNBwAJ
b6a5f4c
to
3cc4352
Compare
@ssteinbach how onerous would it be to split this into two PRs, one that deals with immutability, and one that deals with the float enforcement? |
We could do that. Is that what @reinecke would prefer? My only caveat on that is that there are operations inside OTIO that cast to float - so int-y-ness could get removed anyway. Those come up when you're translating across time rates - if you're using all ints and your time rate is always the same, the addition/subtraction stuff should preserve ints that you put in. |
- Now, if you only provide start_time or duration (not both), the constructor will try and read the rate for the default-constructor created other field from the one you provided - This revealed a bug in one of the files in the unit test suite, which had RV sources with an FPS of 1
0006ab3
to
0541830
Compare
I've rebased to the latest upstream master and removed the cast-to-float. This should be ready to accept. |
Missed this.. This break a lot of my code, as I do += / -= on RationalTime values. |
For the C++ API, we're looking at making the base opentime types immutable (
RationalTime
,TimeRange
, andTimeTransform
).This also includes a subtle fix to the way that
TimeRange
is constructed: now if you pass in only one of the two arguments, the other argument will be initialized in the rate of the one you passed in.