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

Updated Effect schema with enabled flag #1798

Conversation

fschleich
Copy link
Contributor

@fschleich fschleich commented Sep 27, 2024

This PR addresses #1187 - Add enabled Flag to Effects

If there is an associated issue, link it in the form:

Fixes #1187

Summarize your change.

  • Added enabled flag to Effect schema
    - Updated Effect schema to version 2
    - Updated core version to 0.18.0

Reference associated tests.

  • Updated unit tests in test_effect.py to account for enabled flag
  • added empty_effect.json and updated test_json_backend.py accordingly

Copy link

linux-foundation-easycla bot commented Sep 27, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

- Updated Effect schema to version 2 and core version to 0.18.0
- Updated unit tests to account for enabled flag

Signed-off-by: Florian Schleich <[email protected]>
Signed-off-by: Florian Schleich <[email protected]>
@fschleich fschleich force-pushed the schema-update-effects-enabled-flag branch from ca519be to 334bf4c Compare September 27, 2024 17:49
@fschleich
Copy link
Contributor Author

I need some guidance on how to correctly update CORE_VERSION_MAP.cpp, as running make version-map-update results in the below error. Manually setting the version to 0.18.0 results in the failed tests, e.g. here. Would appreciate any pointers or guidance!

(.venv) fschleich@Florians-MacBook-Pro OpenTimelineIO % make version-map-update
updating the CORE_VERSION_MAP...
Traceback (most recent call last):
  File "/Users/fschleich/Developer/github/fschleich/OpenTimelineIO/src/py-opentimelineio/opentimelineio/console/autogen_version_map.py", line 116, in <module>
    main()
  File "/Users/fschleich/Developer/github/fschleich/OpenTimelineIO/src/py-opentimelineio/opentimelineio/console/autogen_version_map.py", line 86, in main
    args = _parsed_args()
           ^^^^^^^^^^^^^^
  File "/Users/fschleich/Developer/github/fschleich/OpenTimelineIO/src/py-opentimelineio/opentimelineio/console/autogen_version_map.py", line 38, in _parsed_args
    default=otio.__version__,
            ^^^^^^^^^^^^^^^^
AttributeError: module 'opentimelineio' has no attribute '__version__'
make: *** [version-map-update] Error 1

@fschleich fschleich marked this pull request as ready for review September 27, 2024 20:28
@jminor
Copy link
Collaborator

jminor commented Sep 30, 2024

A while back when we added enabled to Clip, we were able to do so without increasing the schema version because the new property has a clear fallback value (true). I think you can do the same here for Effect. Here's the PR for Clip for reference: #1175

@codecov-commenter
Copy link

codecov-commenter commented Sep 30, 2024

Codecov Report

Attention: Patch coverage is 94.73684% with 1 line in your changes missing coverage. Please review.

Project coverage is 81.62%. Comparing base (c0e97b0) to head (c2ab22f).
Report is 23 commits behind head on main.

Files with missing lines Patch % Lines
src/opentimelineio/effect.h 50.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1798      +/-   ##
==========================================
- Coverage   84.11%   81.62%   -2.50%     
==========================================
  Files         198      176      -22     
  Lines       22241    12350    -9891     
  Branches     4687     3036    -1651     
==========================================
- Hits        18709    10081    -8628     
+ Misses       2610     1730     -880     
+ Partials      922      539     -383     
Flag Coverage Δ
py-unittests 81.62% <94.73%> (-2.50%) ⬇️

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

Files with missing lines Coverage Δ
src/opentimelineio/effect.cpp 93.33% <100.00%> (+1.66%) ⬆️
...entimelineio-bindings/otio_serializableObjects.cpp 93.69% <100.00%> (-0.17%) ⬇️
.../py-opentimelineio/opentimelineio/schema/effect.py 100.00% <ø> (ø)
tests/test_effect.py 96.42% <100.00%> (+0.35%) ⬆️
tests/test_json_backend.py 93.65% <100.00%> (+0.20%) ⬆️
src/opentimelineio/effect.h 80.00% <50.00%> (-20.00%) ⬇️

... and 65 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5184c36...c2ab22f. Read the comment docs.

@fschleich fschleich force-pushed the schema-update-effects-enabled-flag branch from b266f1a to c2ab22f Compare September 30, 2024 18:06
Copy link
Collaborator

@reinecke reinecke 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 for the update!

@reinecke reinecke merged commit e1f3791 into AcademySoftwareFoundation:main Sep 30, 2024
56 of 57 checks passed
@jmertic jmertic added the devdays24 Dev Days 2024 PRs/Issues label Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants