Move AAF Adapter to separate git repo

[markreidvfx]

Here is a first attempt to move the AAF Adapter to separate git repo.
This started out as a test but turned out to be pretty straightforward,
since I already did most of the work with my AVB adapter.

The extracted AAF Adapter plugin is currently here https://github.com/OpenTimelineIO/otio-aaf-adapter
I did some git magic to preserve the original commit history of the adapter files.

You can use the external adapter without applying this pull request.
Unfortunately using the pkg_resources method doesn't seem to override the contrib plugin.
The OTIO_PLUGIN_MANIFEST_PATH env variable is needed in that case.

The adapter also uses features that aren't in the 0.14 release, and doesn't work with a pypi wheel.

[codecov-commenter]

Codecov Report

Merging #1348 (2f3d0de) into extract_adapters (d1608c7) will decrease coverage by 1.11%.
The diff coverage is n/a.

Additional details and impacted files

@@                 Coverage Diff                  @@
##           extract_adapters    #1348      +/-   ##
====================================================
- Coverage             79.35%   78.24%   -1.12%     
====================================================
  Files                   193      190       -3     
  Lines                 20617    18816    -1801     
  Branches               4152     3846     -306     
====================================================
- Hits                  16361    14722    -1639     
+ Misses                 2145     2031     -114     
+ Partials               2111     2063      -48     

Flag	Coverage Δ
py-unittests	78.24% <ø> (-1.12%)	⬇️

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

Impacted Files	Coverage Δ
src/opentimelineio/generatorReference.h	40.00% <0.00%> (-60.00%)	⬇️
src/opentimelineio/marker.h	83.33% <0.00%> (-16.67%)	⬇️
src/opentimelineio/track.cpp	49.66% <0.00%> (-2.02%)	⬇️
src/opentimelineio/composition.cpp	46.62% <0.00%> (-0.97%)	⬇️
...entimelineio-bindings/otio_serializableObjects.cpp	36.09% <0.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 d1608c7...2f3d0de. Read the comment docs.

[meshula]

It's nice to see that it was easily separable! Thanks for getting the ball rolling.

Perhaps it would make sense to also add a line to Readme.md in the "Adapters" section, that tells where to find the relocated AAF adapter. When this is ready to land, we could update the Readme.md to point to a fork of your adapter work into the OpenTimelineIO org, as my working assumption is that this adapter will fall in the "officially supported by the OTIO project" umbrella.

[markreidvfx]

Good idea. The api docs for adapter aren't being generated anywhere yet either. I'm looking into next too.

[JeanChristopheMorinPerso]

It looks like the coverage decreases with this PR. For example https://app.codecov.io/gh/AcademySoftwareFoundation/OpenTimelineIO/compare/1348?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr%20comments&utm_term=None#D8L191 isn't covered anymore. https://app.codecov.io/gh/AcademySoftwareFoundation/OpenTimelineIO/compare/1348?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr%20comments&utm_term=None#D1L172 too.

When moving adapters, wi'll have to be carefull to keep the code coverage at a good level since some code seems to be only covered by adapter tests.

[meshula]

Every decrease noted indicates a missing unit test on the C/C++ side of things. Some strategies are to (1) add unit tests as part of the change, (2) start an Issue that lists with checkboxes missing unit tests corresponding to items identified by the CI, and start a separate effort to improve test coverage, (3) leave test coverage for future us.

I'm personally inclined to (2), but would be interested in hearing other people's thoughts about it. The reason I'm inclined to (2) is that my preference would be that good unit tests are written, rather than letter of the law unit tests that simply satisfy a bot, and good unit tests could stall things indefinitely until someone has time to devote to it.

[markreidvfx]

It looks pretty straight forward to add a few tests to restore coverage. I will send a pull request.

[reinecke]

This (and the forked repo you created) is really great!
On the unit test front, I think we should emphasize good tests being written and minimize the burden on these kinds of PRs getting through, so somehow noting the changes for future and letting this get through feels like the best approach (another vote for (2) in the suggestions @meshula made). I think it probably should fall on us maintainers to own doing the work associated with that.

On the adapter precedence front - this is an issue I've run into as well. In my case I was able to work around it by giving the adapter a different name and using otio.adapters.from_name - but I don't think this is the right approach for this case. My instinct is that we should at least make the pkg_resources plugins take precedence. This does open the question of what happens if you have multiple pkg_resources plugins registered for the same extension. However I think that case will be a bit rarer, is currently an issue anyway, and can be managed by users when choosing which python packages to install.

[markreidvfx]

I did add the unit tests that restore the coverage lost from the adapter removal. I was hoping to remove it from this pull and create separate pull request, but first wanted to demonstrate it fixes the issue.

[markreidvfx]

I'm unsure why the EDL adapter has shown up in the Codecov report? I think its something to do with the recent EDL commits. The Codecov website can be pretty frustrating to use :p. By my account though, the added unit tests should restore the coverage lost from the adapters removal and even add a bit more.

[JeanChristopheMorinPerso]

I think codecov is just confused. Codecov is unfortunately quite easy to confuse.

[meshula]

Tests are awesome :) Why don't we go ahead and PR/land them separately, per your suggestion? I think they're ready.

[markreidvfx]

Will do

[markreidvfx]

The separated AAF Adapter is now located here
https://github.com/OpenTimelineIO/otio-aaf-adapter

I created a task list of things that need to happen before this pull request could safely be applied
OpenTimelineIO/otio-aaf-adapter#1

Please let me know if there are any other blocking issues to add to this task list.

[jminor]

@markreidvfx could you aim this PR at the newly created extract_adapters branch? I think there should be an "Edit" button at the top of the PR page for this which will let you change the target branch.

[markreidvfx]

@jminor the pull has be change to target that branch. There were some conflicts which I've resolved. f7123d0 made some changes to the aaf adapter, I'll port them to the adapter repo.

[apetrynet]

@markreidvfx, would you mind rebasing on top of or merging the latest changes in the "extract_adapters"? It might kick off the CI so we can see that everything is still good to go.

[markreidvfx]

@apetrynet rebased and all tests pass!

[apetrynet]

Thanks @markreidvfx!