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

Unifies ALE to the EDL adapter by storing the cdl data in the metadata #621

Merged

Conversation

@vvzen
Copy link
Contributor

vvzen commented Nov 21, 2019

Minor fix to the ALE adapter so that it stores the cdl data in the cdl property of the metadata dictionary, in the same way that the EDL one does.
It also adds one more ALE sample and related unittest.

Related issue: #619

structure. Add one more test file and test case.
Copy link
Collaborator

reinecke left a comment

Looks reasonable to me. Just the one comment about checking the CDL values in the unittest.

@@ -98,6 +99,37 @@ def test_ale_read2(self):
]
)

def test_ale_read3(self):

This comment has been minimized.

Copy link
@reinecke

reinecke Nov 22, 2019

Collaborator

Would you mind adding a check in the tests to make sure the CDL data comes through correctly?

Also, if you like, you can rename this to something like test_ale_read_cdl to be explicit about the case this particular test is checking.

This comment has been minimized.

Copy link
@vvzen

vvzen Nov 23, 2019

Author Contributor

Thanks God you actually asked me to do that! :D
I found that I had swapped the offset and power values (it literally slipped through my eyes).
I added the cdl test. Let me know.

… and power were flipped). Add unit test for cdl values.
@vvzen vvzen force-pushed the vvzen:unify-ale-adapter-to-edl-cdl branch from 8359f15 to 9c55267 Nov 23, 2019
Valerio Viperino
@vvzen vvzen force-pushed the vvzen:unify-ale-adapter-to-edl-cdl branch from a9a6f63 to 7eee1ea Nov 25, 2019
Copy link
Collaborator

reinecke left a comment

Looking really good!

Just the one note about unicode and these lint issues to address:

./contrib/opentimelineio_contrib/adapters/ale.py:157:1: E302 expected 2 blank lines, found 1
./contrib/opentimelineio_contrib/adapters/ale.py:180:1: E302 expected 2 blank lines, found 1

(you can also verify this locally by doing make lint from within the OTIO root dir)

contrib/opentimelineio_contrib/adapters/ale.py Outdated Show resolved Hide resolved
…finition, as required by pep8.
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Nov 27, 2019

Codecov Report

Merging #621 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #621   +/-   ##
=======================================
  Coverage   81.69%   81.69%           
=======================================
  Files          72       72           
  Lines        2732     2732           
=======================================
  Hits         2232     2232           
  Misses        500      500
Flag Coverage Δ
#py27 81.67% <ø> (ø) ⬆️
#py36 81.67% <ø> (ø) ⬆️
#py37 81.67% <ø> (ø) ⬆️

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 6d32687...c5be8ea. Read the comment docs.

if 'CDL' in metadata:
cdl = _cdl_values_from_metadata(metadata['CDL'])
if cdl:
metadata.pop('CDL')

This comment has been minimized.

Copy link
@jminor

jminor Dec 2, 2019

Collaborator

It is slightly more pythonic to use del metadata['CDL'] instead of pop

This comment has been minimized.

Copy link
@vvzen

vvzen Dec 4, 2019

Author Contributor

Cool! Makes sense since we don't need the returned index

metadata.pop('ASC_SOP')

if 'ASC_SAT' in metadata:
if metadata['ASC_SAT']:

This comment has been minimized.

Copy link
@jminor

jminor Dec 2, 2019

Collaborator

You could combine these two ifs into one like this: if metadata.get('ASC_SAT') since get will return None if the key is missing.

This comment has been minimized.

Copy link
@vvzen

vvzen Dec 4, 2019

Author Contributor

Yup, I generally use the in keyword because it should perform in constant time since it performs an hashing function, while I don't know if get() internally tries to get the key and catches a KeyError (which is generally slower). But yeah, no harm in just using get I guess!

contrib/opentimelineio_contrib/adapters/ale.py Outdated Show resolved Hide resolved
@vvzen

This comment has been minimized.

Copy link
Contributor Author

vvzen commented Dec 16, 2019

Any updates on this one? :)

@jminor
jminor approved these changes Dec 16, 2019
@jminor
jminor approved these changes Dec 18, 2019
@jminor

This comment has been minimized.

Copy link
Collaborator

jminor commented Dec 18, 2019

@reinecke can you look again? I think all your review notes were addressed.

@vvzen vvzen requested a review from reinecke Jan 12, 2020
Copy link
Collaborator

reinecke left a comment

Looks good, thanks for the fix!

@reinecke reinecke merged commit 6d9a10f into PixarAnimationStudios:master Jan 13, 2020
1 check passed
1 check passed
Travis CI - Pull Request Build Passed
Details
@jminor jminor added this to In progress in Schema Development via automation Jan 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Schema Development
  
In progress
4 participants
You can’t perform that action at this time.