-
Notifications
You must be signed in to change notification settings - Fork 299
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
Add convenience functions #1825
Add convenience functions #1825
Conversation
Signed-off-by: Darby Johnston <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
❌ Your changes status has failed because you have indirect coverage changes. Learn more about Unexpected Coverage Changes and reasons for indirect coverage changes. Additional details and impacted files@@ Coverage Diff @@
## main #1825 +/- ##
==========================================
+ Coverage 84.11% 84.80% +0.69%
==========================================
Files 198 176 -22
Lines 22241 12745 -9496
Branches 4687 1181 -3506
==========================================
- Hits 18709 10809 -7900
+ Misses 2610 1758 -852
+ Partials 922 178 -744
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 124 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
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.
good additions, some requests
Signed-off-by: Darby Johnston <[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.
thanks for the update!
I had to think for a moment about the duration<0 check... To be clear, it should be fine for a RationalTime to have a negative value (e.g. when subtracting two times |
What do you think about having invalid time and range member variables? That way you can use them to initialize values like this:
Which I think is a bit more readable and consistent than constructing invalid times manually. There is an |
Signed-off-by: Darby Johnston <[email protected]>
I'm a little hesitant about that since people might think |
That's a good point. I find the |
Equality versus strict equality, both are useful... |
Summarize your change.
Add a few convenience functions:
TimeRange(0.0, 48.0, 24.0)
, which is a bit more convenient than:TimeRange(RationalTime(0.0, 24.0), RationalTime(48.0, 24.0))
.RationalTime::is_valid_time()
to complementRationalTime::is_invalid_time()
. This can be a bit easier to read:if (time.is_valid_time())
, instead of:if (!time.is_invalid_time())
.TimeRange::is_invalid_range()
andTimeRange::is_valid_range()
to match RationalTime.Small fix: