-
Notifications
You must be signed in to change notification settings - Fork 10
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
Extend and improve acre #2
base: master
Are you sure you want to change the base?
Conversation
env_prototype/core.py
Outdated
return None | ||
|
||
|
||
def execute(executable, args=None, environment=None, cwd=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.
What's the reason for refactoring this to execute
. It's the same method as in Avalon pipeline currently, but there it's called launch? Any reason why the original was a bad name?
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.
@aardschok should we still refactor this to launch
?
Still not too sure about having the |
I think we can rename |
Tested it for Maya and Fusion deadline submission, had to add |
I do not think that we should care about familiarity of others when using this. We could have chosen Let's tackle it from a pure logical standpoint without taking in account the OS in which it is used.
Now based on pure grammar: we already KNOW the object, we pass it on to the function. Taking that logic in account both I am entirely in favor to use |
No, The name which comes from the fact that the same executable name can be on your path, e.g. python.exe in Python3 and Python2. You use Again, subtle difference. I'm fine with |
Any objection to merging this? |
acre/launch.py
Outdated
import os | ||
import pprint | ||
|
||
from . import discover, build, merge, locate, launch |
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.
Is this importing itself? Isn't this a naming conflict? :) Interesting that this is actually allowed.
Other than above note this is fine for me - it's a prototype. Like mentioned in our discussions I think the API can still simplify by just having the By the way, don't you think |
Made some changes to the API, I would like to have a review on the I think that there is still some room for improvements |
Part of issue FUS-32
which
andexecute
function to core