OpenTimelineIO icon indicating copy to clipboard operation
OpenTimelineIO copied to clipboard

Fix parsing of time_strings lacking leading zeroes

Open meshula opened this issue 3 years ago • 1 comments

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.

meshula avatar May 14 '22 03:05 meshula

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%) :arrow_down:

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%) :arrow_down:

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.

codecov-commenter avatar May 14 '22 04:05 codecov-commenter

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%) :arrow_down:

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%) :arrow_down:
tests/test_opentime.py 99.73% <100.00%> (+<0.01%) :arrow_up:

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.

codecov-commenter avatar Sep 12 '22 20:09 codecov-commenter

CLA Signed

The committers listed above are authorized under a signed CLA.

  • :white_check_mark: login: meshula / name: Nick Porcino (b19c7153b3dfe4e0eca77384997ba232cafe5445, 66407368db735fff2f5cdaf460aa8349fcdc3349, 05fdf5fc13c47d22121d530f415066131d860dde)