Fixes #1188 BREAKING CHANGE: change "effect" argument to Gap constructor to "effects"

[visajshah]

This PR fixes #1188.

The argument effect in the Gap constructor has been changed to effects.

[codecov-commenter]

Codecov Report

Merging #1292 (420ed46) into main (1ec6f07) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1292   +/-   ##
=======================================
  Coverage   86.14%   86.14%           
=======================================
  Files         196      196           
  Lines       19813    19813           
  Branches     2314     2314           
=======================================
  Hits        17068    17068           
  Misses       2181     2181           
  Partials      564      564           

Flag	Coverage Δ
py-unittests	86.14% <ø> (ø)

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

Impacted Files	Coverage Δ
...entimelineio-bindings/otio_serializableObjects.cpp	91.78% <ø> (ø)

---

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 1ec6f07...420ed46. Read the comment docs.

[reinecke]

@visajshah Thanks for the fix!

@ssteinbach and @jminor - I have two questions I'd like your opinions on:

1. There apparently aren't any tests that are covering any part of the gap constructor, should there be? Do we want to gate merging this PR on that or take that as a separate task?
2. This is technically a breaking change, but the impact seems pretty low given that effects on a Gap are a somewhat odd case. Are we concerned about that?

My personal opinion is that since this fixes what was likely a typo, it brings the constructor arg on Gap in-line with Item, and the keyword arg is likely rarely used, the breaking change is justified.
On the tests question, I'm a bit more up in the air.

[ssteinbach]

Regarding your questions, @reinecke:

1. There apparently aren't any tests that are covering any part of the gap constructor, should there be? Do we want to gate merging this PR on that or take that as a separate task?

I'm not against adding a test for the gap constructor, but the gap constructor is called, for example:
https://github.com/PixarAnimationStudios/OpenTimelineIO/blob/1ec6f07f1af525ba4ca0aa91e01e5939d6237f01/tests/test_composition.py#L382
Unless someone feels really strongly that it needs to be added, I don't think this PR needs to add a test, unless the author wants to. It would entail adding a test_gap.py with a call to the constructor and pass an argument into the effect parameter.

2. This is technically a breaking change, but the impact seems pretty low given that effects on a Gap are a somewhat odd case. Are we concerned about that?

I think this is ok, I would vote it just needs to be called out in the release notes for the release. We've done similar things before.

My personal opinion is that since this fixes what was likely a typo, it brings the constructor arg on Gap in-line with Item, and the keyword arg is likely rarely used, the breaking change is justified. On the tests question, I'm a bit more up in the air.

My vote is: accept as is, but change the title of the PR to be more representative of the change (which I just did, but feel free to adjust however). Make sure to call out in release notes.

[ssteinbach]

Pending consensus, looks good.

[reinecke]

I think @ssteinbach and I are in alignment on this.
@visajshah We can go ahead and merge this, just let us know whether or not you'd like to add any tests.

[visajshah]

@reinecke, I think we can merge this without adding any new tests.