Skip to content
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 parsing of time_strings lacking leading zeroes #1297

Merged
merged 8 commits into from Sep 15, 2022

Conversation

meshula
Copy link
Collaborator

@meshula meshula commented May 14, 2022

Link the Issue(s) this Pull Request is related to.

#1293

@jchen9 Hi Julian, here's a rework of the existing routine to address the problem you found, and introduce a little more rigor to what is or is not a well-formed time string.

Fixes #1293

Summarize your change.

Describe the reason for the change.

  • fixes parsing of time strings without leading zeroes.
  • enforces that a negative sign can only appear in the left most position
  • implementation does not allocate memory or copy strings
  • implementation does not allow exponential notation and other things that std does allow but are inappropriate for time strings
  • compatible with strings produced by ffprobe

Reference associated tests.

adds C based tests corresponding to the existing Python based tests.

@codecov-commenter
Copy link

Codecov Report

Merging #1297 (219beb0) into main (fb0b86d) will decrease coverage by 0.03%.
The diff coverage is 68.65%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1297      +/-   ##
==========================================
- Coverage   86.14%   86.10%   -0.04%     
==========================================
  Files         196      196              
  Lines       19813    19863      +50     
  Branches     2314     2314              
==========================================
+ Hits        17068    17104      +36     
- Misses       2181     2195      +14     
  Partials      564      564              
Flag Coverage Δ
py-unittests 86.10% <68.65%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/opentime/rationalTime.h 90.00% <ø> (ø)
src/opentime/rationalTime.cpp 81.71% <68.65%> (-3.89%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fb0b86d...219beb0. Read the comment docs.

src/opentime/rationalTime.cpp Outdated Show resolved Hide resolved
tests/test_opentime.cpp Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Sep 12, 2022

Codecov Report

Merging #1297 (26ec46a) into main (64b0e31) will decrease coverage by 0.02%.
The diff coverage is 65.55%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1297      +/-   ##
==========================================
- Coverage   86.15%   86.12%   -0.03%     
==========================================
  Files         199      199              
  Lines       20448    20521      +73     
  Branches     2375     2375              
==========================================
+ Hits        17617    17674      +57     
- Misses       2250     2266      +16     
  Partials      581      581              
Flag Coverage Δ
py-unittests 86.12% <65.55%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/opentime/rationalTime.h 90.00% <ø> (ø)
src/opentime/rationalTime.cpp 81.91% <61.25%> (-3.69%) ⬇️
tests/test_opentime.py 99.73% <100.00%> (+<0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 64b0e31...26ec46a. Read the comment docs.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Sep 13, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

jminor
jminor previously requested changes Sep 13, 2022
src/opentime/rationalTime.cpp Outdated Show resolved Hide resolved
src/opentime/rationalTime.cpp Show resolved Hide resolved
src/opentime/rationalTime.cpp Outdated Show resolved Hide resolved
src/opentime/rationalTime.cpp Outdated Show resolved Hide resolved
@meshula meshula dismissed jminor’s stale review September 14, 2022 05:55

Dismissing this review as it was lost when I rebased this change on latest. The review was addressed though :)

Copy link
Collaborator

@ssteinbach ssteinbach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. One little question about naming. Like to see those c++ tests start to fill in!

src/opentime/rationalTime.cpp Outdated Show resolved Hide resolved
…er portion can be exactly represented in double
Copy link
Collaborator

@ssteinbach ssteinbach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm, thanks!

@meshula meshula merged commit 78941dd into AcademySoftwareFoundation:main Sep 15, 2022
@ssteinbach ssteinbach added this to the Public Beta 15 milestone Sep 19, 2022
@meshula meshula deleted the fix1293 branch September 21, 2022 21:09
MichaelPlug pushed a commit to MichaelPlug/OpenTimelineIO that referenced this pull request Aug 5, 2023
…undation#1297)

Fixes AcademySoftwareFoundation#1293

fixes parsing of time strings without leading zeroes.
enforces that a negative sign can only appear in the left most position
implementation does not allocate memory or copy strings
implementation does not allow exponential notation and other things that std does allow but are inappropriate for time strings
compatible with strings produced by ffprobe
associated tests
adds C based tests corresponding to the existing Python based tests.

Signed-off-by: Michele Spina <michelespina96@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants