Closed darbyjohnston closed 1 year ago
The names each_child
and each_clip
imply a Python iterator. These functions however return collections, not iterators, so they should be renamed
rename each_child
and each_clip
to an appropriate new name.
child_if
is not a Pythonic naming
all_children
implies property access
@meshula When you say that "child_if" is not a Pythonic naming, is there documentation or a style guide that provides guidance on what is and is not proper naming?
@JeanChristopheMorinPerso Following up from our discussion in the PR, if the "all_children" implies a Python property, is there a variation that reads more like a function? Maybe "get_all_children"?
I like get_all_children()
. I think that is clear, easy to remember, and makes sense when read aloud (my personal rule-of-thumb).
Python's standard libraries are somewhat inconsistent in naming conventions, so it is hard to point to a specific rule to follow. The Style Guide for Python doesn't say much on this topic except for this, which aligns with @JeanChristopheMorinPerso 's comment:
Note 2: Avoid using properties for computationally expensive operations; the attribute notation makes the caller believe that access is (relatively) cheap.
For code which does use properties, both the setter and getter tend to have the same name, without any get_
or set_
prefix, but in this case, using the get_
prefix can signal that there is some cost to calling the function, and can make it clear that there isn't a corresponding setter.
Ideally the doc string will clarify that this function gets all of the children recursively whereas the children
property is shallow, and allows both get and set operations.
In that case maybe also get_children_if()
and get_clips_if()
? Or if the get_
prefix is sufficient we could drop the _if
suffix and just have get_children()
and get_clips()
. The presumption being that children
is the lightweight property and get_children()
is a feature rich function with options for recursion and searching within a range.
How do the doc strings work with the Python wrappings?
I like get_all_children
and get_all_clips
I think, but I'm far from a good API designer. As for how docstrings are generated, see https://pybind11.readthedocs.io/en/stable/advanced/misc.html#generating-documentation-using-sphinx. Some examples: https://github.com/AcademySoftwareFoundation/OpenTimelineIO/blob/main/src/py-opentimelineio/opentime-bindings/opentime_rationalTime.cpp#L108-L117.
Thanks for the link to opentime_rationalTime.cpp
, I hadn't noticed those strings before.
Actually how about find_children()
and find_clips()
to replace children_if()
and clip_if()
? It doesn't sound like an attribute since the find_
implies there is some cost involved.
find
also implies an operation that might involve rooting around a bit, which is accurate :)
My thought as well. :) I think it also implies find_
is configurable so it fits nicely with the search_range
and shallow_search
parameters, and possibly additional parameters in the future. Also that find_
could return all, some, or none of the children, in which case maybe the all_children()
function is redundant. It also works with both the C++ and Python API.
I think I prefer find
over get_all
too!
Yes, find_
is a great choice here. 👍
Great, I updated the PR to use find_children
and find_clips
which seems to read nicer than children_if
and clip_if
. I think it also removes the need for the all_
functions, but check out the changes and see what you think.
The deprecated functions are in composition.py, serializable_collection.py, stack.py, timeline.py, and track.py.
Calls to these functions can be replaced with the new "children_if()" and "clip_if()" functions.