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

[Breaking change] Remove support for passing a None value as the name argument #1453

Conversation

JeanChristopheMorinPerso
Copy link
Member

@JeanChristopheMorinPerso JeanChristopheMorinPerso commented Oct 15, 2022

Remove support for passing a None value as the name argument to Marker, Track, Stack and MissingReference constructors

Follow up to #1446.

As discussed in the last TSC meeting, it has been agreed that we shuld keep the class constructors consistent, which mean that we should drop the ability to pass None values for the name argument of the Marker, Track, Stack and MissingReference class constructors.

Passing None is bad in this case because it's inconsistent with the other constructors, which can be seen as not user friendly.

The name parameter is always defined as a kwarg with a default value of '' (empty string). This means that it's still possible to ignore the name parameter entirely when instantiating a class. The only difference now is that the type will be explicitly exposed (and discoverable) and if something else than a string is passed as an argument, it will cause an exception like this:

Traceback (most recent call last):
  File "<string>", line 1, in <module>
TypeError: __init__(): incompatible constructor arguments. The following argument types are supported:
    1. opentimelineio._otio.Marker(name: str = '', marked_range: opentimelineio._opentime.TimeRange = otio.opentime.TimeRange(start_time=otio.opentime.RationalTime(value=0, rate=1), duration=otio.opentime.RationalTime(value=0, rate=1)), color: str = 'RED', metadata: object = None)

Invoked with: kwargs: name=None

This is considered a breaking change and should be clearly marked as such in the changelog.

…r, Track, Stack and MissingReference constructors

Signed-off-by: Jean-Christophe Morin <[email protected]>
@codecov-commenter
Copy link

Codecov Report

Merging #1453 (788a193) into main (c2507e1) will increase coverage by 0.00%.
The diff coverage is 33.33%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1453   +/-   ##
=======================================
  Coverage   78.88%   78.89%           
=======================================
  Files         200      200           
  Lines       21728    21724    -4     
  Branches     4406     4403    -3     
=======================================
- Hits        17141    17140    -1     
  Misses       2354     2354           
+ Partials     2233     2230    -3     
Flag Coverage Δ
py-unittests 78.89% <33.33%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
...entimelineio-bindings/otio_serializableObjects.cpp 26.59% <0.00%> (+0.01%) ⬆️
...-opentimelineio/opentimelineio/adapters/fcp_xml.py 88.40% <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 c2507e1...788a193. Read the comment docs.

@ssteinbach ssteinbach added this to the Public Beta 16 milestone Oct 18, 2022
@ssteinbach ssteinbach merged commit d59a358 into AcademySoftwareFoundation:main Oct 18, 2022
@JeanChristopheMorinPerso JeanChristopheMorinPerso deleted the remove_optional_names branch October 18, 2022 23:13
MichaelPlug pushed a commit to MichaelPlug/OpenTimelineIO that referenced this pull request Aug 5, 2023
…r, Track, Stack and MissingReference constructors (AcademySoftwareFoundation#1453)

Signed-off-by: Jean-Christophe Morin <[email protected]>
Signed-off-by: Michele Spina <[email protected]>
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.

4 participants