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

Add to_dict methods #1705

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

JeanChristopheMorinPerso
Copy link
Member

@JeanChristopheMorinPerso JeanChristopheMorinPerso commented Feb 29, 2024

Add to_dict and to_list methods to more easily pass OTIO objects to json.dumps.

This is the result from https://academysoftwarefdn.slack.com/archives/CMQ9J4BQC/p1708976729390279.

…o_list to AnyVector. Useful to pass to json.dumps.

Signed-off-by: Jean-Christophe Morin <[email protected]>
Signed-off-by: Jean-Christophe Morin <[email protected]>


@add_method(AnyDictionary)
def to_dict(self): # noqa: F811
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not convinced that this is the best name. Should we instead call it to_py? It would be more generic. I don't like that we have to_dict on everything except AnyVector which has to_list.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to_json?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no type named json in Python. to_json to me means "to JSON formatted string". And it's not json specific, it's really just a pure python data structure.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK I see what you mean. to_py is indeed more generic then, even if it's a bit less intuitive?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to_dict and to_list over to_py.
That way you know what you get.
Of course the nature of the object will indicate what to_py returns, but I still prefer the explicitness of to_dict and to_list

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't mind using to_dict because I'm used to seeing and using this to make a complex structure a simple dictionary. To me it makes sense.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to_py would allow us to iterate through a list of OTIO objects (of any type) and convert them to python types without using isinstance to pick the right method.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow up to this, I ended up having to make a helper function to do exactly this at work. I ended up calling the function pythonify to communicate its returning Python objects.

to_py seems in line with other conversion method names on OTIO objects and is clear of what to expect. Another reason to use a py/python name is because AnyDictionary and AnyVector can nest each other so you're not just getting a dict or list but rather the entire struct is pythonify-ed.

Signed-off-by: Jean-Christophe Morin <[email protected]>
@JeanChristopheMorinPerso JeanChristopheMorinPerso marked this pull request as ready for review March 9, 2024 01:00
@JeanChristopheMorinPerso
Copy link
Member Author

This is ready to review. I would need help with the model regeneration. I'm not too sure what's going on with that.

@reinecke
Copy link
Collaborator

I think we're thumbsup on adding this. The test failure is a bit of a head-scratcher - we'll need to look in this vicinity to determine why the module path is coming out wrong in the docs.

@apetrynet
Copy link
Contributor

apetrynet commented Mar 28, 2024

If this is doc related it might be that you'll need to run make doc-plugins-update locally and push again.
Ran into a similar issue on the adapter breakouts.

Copy link
Collaborator

@meshula meshula left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks good to me assuming the CI failures are unrelated to the implementation itself.

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.

6 participants