Replace py::object type by optional<std::string> for name parameters

[JeanChristopheMorinPerso]

Summarize your change.

Yet another type improvement. This time, I'm changing the name parameters to be explicit about which types they accept. They accept either None or a string.

Note that this has a side effect. Before, the name parameters would accept anything (because of py::object), which means that passing a dict would produce "{}" for example. I think it's fine if we stop supporting this as it was probably not intentional in the first place.

Question for the TSC: Some constructors use std::string for the name parameter while some others accept None or string. Should we simply just remove support for None? That would simplify things and also make it consistent across constructors.

[meshula]

Are you asking, specifically with regards to this PR, should we replace the optional<std::string> with std::string, or are you asking a broader question across the entire OTIO API?

[codecov-commenter]

Codecov Report

Merging #1446 (8854598) into main (0763bf6) will increase coverage by 0.00%.
The diff coverage is 33.33%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1446   +/-   ##
=======================================
  Coverage   78.83%   78.84%           
=======================================
  Files         203      203           
  Lines       22252    22248    -4     
  Branches     4521     4519    -2     
=======================================
- Hits        17543    17542    -1     
  Misses       2437     2437           
+ Partials     2272     2269    -3     

Flag	Coverage Δ
py-unittests	78.84% <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	24.54% <33.33%> (-0.19%)	⬇️
...ntimelineio/opentimelineio-bindings/otio_tests.cpp	29.78% <0.00%> (+1.06%)	⬆️

---

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 0763bf6...8854598. Read the comment docs.

[JeanChristopheMorinPerso]

Are you asking, specifically with regards to this PR, should we replace the optional<std::string> with std::string, or are you asking a broader question across the entire OTIO API?

Specifically with regards to this PR.

[meshula]

Ok, my first question would be if you make it not-optional, would there be a lot of work on adapters and other py based tools to introduce names where maybe there are none presently?

[JeanChristopheMorinPerso]

When I remove the optional, it looks like we only need to do a small change:

diff --git a/src/py-opentimelineio/opentimelineio/adapters/fcp_xml.py b/src/py-opentimelineio/opentimelineio/adapters/fcp_xml.py
index 98e1fd1..4d60e3a 100644
--- a/src/py-opentimelineio/opentimelineio/adapters/fcp_xml.py
+++ b/src/py-opentimelineio/opentimelineio/adapters/fcp_xml.py
@@ -738,7 +738,7 @@ class FCP7XMLParser:
         """
         local_context = context.context_pushing_element(track_element)
         name_element = track_element.find("./name")
-        track_name = (name_element.text if name_element is not None else None)
+        track_name = (name_element.text if name_element is not None else '')
 
         timeline_item_tags = {"clipitem", "generatoritem", "transitionitem"}

At least all tests are passing with this change.

name is already a kwarg with a default of ''. I think that disallowing None would be a safe change to do.

[meshula]

ok, sounds good to me.

[JeanChristopheMorinPerso]

@ssteinbach @meshula what should we do about the inconsistency of the name parameter type (sometimes optional and sometimes string)?

[reinecke]

From TSC meeting: The change of no longer accepting None for these sounds okay - we just need to document in the release notes that this will be a breaking change for people.

[JeanChristopheMorinPerso]

Perfect, I'll create a PR later this week. Thanks!