-
Notifications
You must be signed in to change notification settings - Fork 49
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
fix #406: Unified attributes names #413
Conversation
avalon/maya/commands.py
Outdated
edit_in = shot["data"]["edit_in"] | ||
edit_out = shot["data"]["edit_out"] | ||
|
||
frame_start = shot["data"].get("frameStart", shot["data"]["edit_in"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is wrong. :)
Unlike an or
statement I believe Python does not know how to optimize the out the second argument as such the full shot["data"]["edit_in"]
is always retrieved. As susch, whenever only frameStart
is in the data this code will still raise a KeyError:
data = {"frameStart": 1}
print data.get("frameStart", data["edit_in"])
# KeyError
print data.get("frameStart", data.get("edit_in", None))
# Yay! :)
avalon/tools/cbloader/model.py
Outdated
end = version_data.get("endFrame", None) | ||
handles = version_data.get("handles", None) | ||
if start is not None and end is not None: | ||
frame_start = version_data.get("frameStart", None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fails to fall back to the original startFrame
and thus is not backwards compatible.
This should be:
version_data.get("frameStart",
# backwards compatibility
version_data.get("startFrame", None))
The same goes for frameEnd
below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true. well spotted.
|
||
frame_start = shot["data"].get( | ||
"frameStart", | ||
shot["data"].get("edit_in") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider this a question, but should we maybe explicitly comment with these fallbacks that they are due to "Backwards compatibility" - I can imagine newcomers or ourselves in 1-2 years looking at this code and be like "why the hell does it also allow edit_in
?"
I'd rather have it clear that it's only allowing that fallback due to backwards compatibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can really imagine myself in 1-2 weeks looking at this code and be like "who the hell wrote this?" Agree
Note to everyone reading this. If you have Loader plug-ins or other features in your own pipeline relying on start frame or end frame, e.g. a "Set Time Range" based on published data make sure to include the backwards compatibility and gently move forward to the new unified naming conventions. ❤️ |
Flawless, looking good now. Will try to roll this out tomorrow and test it. :) Nice work! |
@mottosso any objections? If not, then this is ready to merge I believe |
…fterEffects-to-OpenPype AfterEffects: Move implementation to OpenPype
Changed attribute naming:
edit_in
,startFrame
changed toframeStart
edit_out
,endFrame
changed toframeEnd
resolution_width
changed toresolutionWidth
resolution_height
changed toresolutionHeight
handles
can be stored ashandleStart
andhandleEnd
Reference issue: #406
Everything should stay backwards compatible