Add to_dict methods#1705
Add to_dict methods#1705JeanChristopheMorinPerso wants to merge 6 commits intoAcademySoftwareFoundation:mainfrom
Conversation
…o_list to AnyVector. Useful to pass to json.dumps. Signed-off-by: Jean-Christophe Morin <jean_christophe_morin@hotmail.com>
|
|
||
|
|
||
| @add_method(AnyDictionary) | ||
| def to_dict(self): # noqa: F811 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
OK I see what you mean. to_py is indeed more generic then, even if it's a bit less intuitive?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Can we take some naming inspiration from @dataclass or some other plain-old-data terminology?
JeanChristopheMorinPerso
force-pushed
the
add_to_dict_methods
branch
from
413f418 to
2c0612e
Compare
Signed-off-by: Jean-Christophe Morin <jean_christophe_morin@hotmail.com>
JeanChristopheMorinPerso
force-pushed
the
add_to_dict_methods
branch
from
2c0612e to
d19d487
Compare
|
This is ready to review. I would need help with the model regeneration. I'm not too sure what's going on with that. |
|
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. |
|
If this is doc related it might be that you'll need to run |
meshula
left a comment
meshula
left a comment
There was a problem hiding this comment.
this looks good to me assuming the CI failures are unrelated to the implementation itself.
jminor
added
help wanted
8 participants
Add
to_dictandto_listmethods to more easily pass OTIO objects tojson.dumps.This is the result from https://academysoftwarefdn.slack.com/archives/CMQ9J4BQC/p1708976729390279.