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

ImageSequenceReference: Added handling for empty target_url_base #891

Conversation

reinecke
Copy link
Collaborator

Fixes #890

Summarize your change.

There was a check to see if target_url_base ended in a / to avoid having double-/ in constructed urls. This assumed `target_url_was of at lease length 1. Added a check to not attempt the check if the length is zero.

Reference associated tests.

Added test checking generation of bare filenames using target_url_for_image_number.

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.

I think we're using unittest style assertions, does this change make sense?

),
)

assert ref.target_url_for_image_number(0) == 'myfilename.0101.exr'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
assert ref.target_url_for_image_number(0) == 'myfilename.0101.exr'
self.assertEquals(ref.target_url_for_image_number(0), 'myfilename.0101.exr')

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

🤦 pytest muscle memory. Fixed.

@codecov-io
Copy link

codecov-io commented Feb 20, 2021

Codecov Report

Merging #891 (5309eef) into master (06dc494) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #891   +/-   ##
=======================================
  Coverage   86.65%   86.65%           
=======================================
  Files         183      183           
  Lines       17879    17883    +4     
  Branches     1972     1972           
=======================================
+ Hits        15493    15497    +4     
  Misses       1902     1902           
  Partials      484      484           
Flag Coverage Δ
unittests 86.58% <100.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
src/opentimelineio/imageSequenceReference.cpp 93.50% <100.00%> (+0.08%) ⬆️
tests/test_image_sequence_reference.py 100.00% <100.00%> (ø)

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 06dc494...5309eef. Read the comment docs.

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 great! Thanks!

@ssteinbach ssteinbach merged commit 5005650 into AcademySoftwareFoundation:master Feb 20, 2021
@ssteinbach ssteinbach added this to the Public Beta 14 milestone Apr 12, 2021
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.

ImageSequenceReference: If target_url_base is empty string, target_url_for_image_number raises IndexError
3 participants