[RodeoFX] Fix Marker __repr__ and __str__ #592

[elabrosseRodeofx]

Fixes: #592

This PR fixes an AttributeError when calling
repr(otio.schema.Marker()) or str(otio.schema.Marker())

Without the fix:

>>> repr(otio.schema.Marker())
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/devLocal/elabrosse/rdoenv/auto_lineup/otio/lib/python2.7/site-packages/opentimelineio/schema/marker.py", line 25, in __repr__
    repr(self.media_reference),
AttributeError: 'opentimelineio._otio.Marker' object has no attribute 'media_reference'

>>> str(otio.schema.Marker())
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/devLocal/elabrosse/rdoenv/auto_lineup/otio/lib/python2.7/site-packages/opentimelineio/schema/marker.py", line 9, in __str__
    self.media_reference,
AttributeError: 'opentimelineio._otio.Marker' object has no attribute 'media_reference'

With the fix:

>>> repr(otio.schema.Marker())
"otio.schema.Marker(name='', marked_range=otio.opentime.TimeRange(start_time=otio.opentime.RationalTime(value=0, rate=1), duration=otio.opentime.RationalTime(value=0, rate=1)), metadata={})"

>>> str(otio.schema.Marker())
'Marker("", TimeRange(RationalTime(0, 1), RationalTime(0, 1)), {})'

The otio.schema.Marker object's __repr__ and __str__ methods are referencing a 'media_reference' and a 'source_range' attribute.
however Markers do not have those attributes. They do however have a 'marked_range' attribute that is not referenced in the __repr__ / __str__

[jminor]

Could you also include the marker's color? It looks like we forgot that also.

[ssteinbach]

Awesome, thanks!

If you say:
Fixes: #592
instead of
Issue: #592

Then when this PR is accepted github will automatically close the issue for us, too.

[ssteinbach]

Looks good, but would you mind adding a repr/str test for us too?

Like this:
https://github.com/PixarAnimationStudios/OpenTimelineIO/blob/74c09514ddc240bce71ff9fde4dda3cc1c9ffe4a/tests/test_media_reference.py#L51

But add it to the file test_marker.py:
https://github.com/PixarAnimationStudios/OpenTimelineIO/blob/master/tests/test_marker.py

Thanks! Good catch.

[jminor]

It looks like the Python 3 tests are failing because there's a subtle difference in how unicode is handled between Python 2 and 3. You can try just removing the u from this line to see if that avoids the issue.

[elabrosseRodeofx]

If I remove the u from the string the python-3 tests will pass but the python-2 tests will fail.

I dug a little bit and it seems that the marker's color property returns a unicode string in python-2 since opentimelineio-0.12. It used to return a native string.

otio-0.11, python2.7
>>> m = otio.schema.Marker()

>>> m.color
'RED'

otio-0.12, python2.7
>>> m = otio.schema.Marker()

>>> m.color
u'RED'

I feel like the color property should always return a native python string. What do you think ?

I also noticed something similar with the metadata property. It seems that if a string value is added to an anyDictionary object, it is converted to a unicode string.

-0.11, python2.7
>>> m = otio.schema.Marker(metadata={'foo': 'bar'})

>>> m.metadata
{'foo': 'bar'}

>>> m.metadata['spam'] = 'egg'
>>> m.metadata
{'foo': 'bar', 'spam': 'egg'}

-0.12, python2.7
>>> m = otio.schema.Marker(metadata={'foo': 'bar'})

>>> m.metadata
{'foo': u'bar'}

>>> m.metadata['spam'] = 'egg'
>>> m.metadata
{'foo': u'bar', 'spam': u'egg'}

as you can see the values are converted to unicode strings even though they were explicitly native strings.

I was able to fix this on my end. I'd like to make an issue/PR to propose the change. Do you think this is indeed an issue that should be fixed ? If both issues were fixed then we could remove the u from the test string and it would pass for both python-2 and 3.

Otherwise I could use a different test string depending on the python major version but it feels like a workaround rather than a real fix.

Let me know what you think !

[jminor]

@reinecke do you have any advice about how to make this work in both Python 2 and 3? Marker colors will likely stay as plain ASCII, but metadata should allow for unicode characters.

[jminor]

This might help illuminate the different handling of str/unicode in Python 2 vs 3: https://portingguide.readthedocs.io/en/latest/strings.html

[reinecke]

My current guess on this is that it has to do with the marshaling behavior in pybind 11: https://pybind11.readthedocs.io/en/stable/advanced/cast/strings.html
Given that, it appears all strings should become unicode upon transfer back into python.

So, for python 2, it seems like we should at least be able to rely on the u being added consistently? Unfortunately I can't think of a clean way of branching the literals we compare based on the two python versions.

Would it make sense to just include two copies of the repr unittests (one marked skip in python3 and one marked skip in python2)? Then when python2 support is dropped we can remove the tests marked skip in python3 and delete the skip annotations.

An alternate approach could be to have a dedicated custom assertion that when run in python2 compares against a "patched" version of the string using something like comp_string.replace("u'", "'").replace('u"', '"'), but that feels pretty sketchy. The upside is that we could make it be a no-op in the python3 use case and the assertion could be find and replaced upon dropping python 2 support.

For reference, here is a good way to mark the unittests:

IS_PYTHON_2 = (sys.version_info < (3, 0))

@unittest.skipIf(IS_PYTHON_2, "In python2 string values become u-strings.")
def test_repr():
    ...do stuff...

@unittest.skipIf(not IS_PYTHON_2, "In python2 string values become u-strings.")
def test_repr_py2():
    ...do stuff...

[reinecke]

Accidentally re-resolved in #740 (even though this was the original fix).