-
Notifications
You must be signed in to change notification settings - Fork 31
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
Refactor the frontend. #89
Conversation
1ef45fe
to
a083596
Compare
if api_version >= '2.1': | ||
from ambuild2.frontend.v2_1.context_manager import ContextManager | ||
elif api_version >= '2.0': | ||
from ambuild2.frontend.v2_0.context_manager import ContextManager |
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 have a better import suggestion:
if api_version >= '2.1': | |
from ambuild2.frontend.v2_1.context_manager import ContextManager | |
elif api_version >= '2.0': | |
from ambuild2.frontend.v2_0.context_manager import ContextManager | |
ContextManager = __import__('ambuild2.frontend.v{0}.context_manager'.format(api_version.replace('.', '_')), | |
fromlist=('*',)).ContextManager |
Idk if it's format correct though.
if 'originalCwd' in self.vars: | ||
originalCwd = self.vars['originalCwd'] | ||
else: | ||
originalCwd = self.vars['sourcePath'] |
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.
'self.vars' is just a dict
no? If so, this is simpler to read is it not?
if 'originalCwd' in self.vars: | |
originalCwd = self.vars['originalCwd'] | |
else: | |
originalCwd = self.vars['sourcePath'] | |
originalCwd = self.vars.get('originalCwd', self.vars['sourcePath']) |
|
||
@property | ||
def backend(self): | ||
raise Exception('Must be implemented!') |
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.
If we were using pylint for linting, you'll be chewed out for using Exception
raise Exception('Must be implemented!') | |
raise NotImplementedError('Must be implemented!') |
Infact, we can just turn this into an instance of ABCMeta
no?
Though to do that, we require the "six" module.
raise Exception('Must be implemented!') | ||
|
||
def addSymlink(self, context, source, output_path): | ||
raise Exception('Must be implemented!') |
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.
raise Exception('Must be implemented!') | |
raise NotImplementedError('Must be implemented!') |
raise Exception('Must be implemented!') | ||
|
||
def addFolder(self, context, folder): | ||
raise Exception('Must be implemented!') |
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.
raise Exception('Must be implemented!') | |
raise NotImplementedError('Must be implemented!') |
raise Exception('Must be implemented!') | ||
|
||
def addCopy(self, context, source, output_path): | ||
raise Exception('Must be implemented!') |
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.
raise Exception('Must be implemented!') | |
raise NotImplementedError('Must be implemented!') |
weak_inputs = [], | ||
shared_outputs = [], | ||
env_data = None): | ||
raise Exception('Must be implemented!') |
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.
raise Exception('Must be implemented!') | |
raise NotImplementedError('Must be implemented!') |
raise Exception('Must be implemented!') | ||
|
||
def addConfigureFile(self, context, path): | ||
raise Exception('Must be implemented!') |
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.
raise Exception('Must be implemented!') | |
raise NotImplementedError('Must be implemented!') |
When it was time to do the 2.1 API, I figured it'd be good to simply copy and paste the entire frontend to minimize compatibility issues. That doesn't really work because a good deal of code (eg the database) is shared, so really everything in one API has to be sensitive to the overall system. Also, a great deal of delicate code (amb2.py, for example), winds up duplicated with very little changes. This patch splits the API-specific stuff out of generators, so we can reduce the number of generators from 4 to 2. The API specific parts have been renamed to what they actually are: ContextManagers. Some of that can be shared and now exists in a base class, but most of the details of context management will remain API specific. An amount of the C++-specific stuff has been moved out of generators as well, since this tended to be API specific. While we're at it, this also simplifies the directory structure a bit.
Thanks for the tips, I'll keep this in mind as I rejigger stuff for 2.2. |
When it was time to do the 2.1 API, I figured it'd be good to simply
copy and paste the entire frontend to minimize compatibility issues.
That doesn't really work because a good deal of code (eg the database)
is shared, so really everything in one API has to be sensitive to the
overall system. Also, a great deal of delicate code (amb2.py, for
example), winds up duplicated with very little changes.
This patch splits the API-specific stuff out of generators, so we can
reduce the number of generators from 4 to 2. The API specific parts have
been renamed to what they actually are: ContextManagers. Some of that
can be shared and now exists in a base class, but most of the details of
context management will remain API specific.
An amount of the C++-specific stuff has been moved out of generators as
well, since this tended to be API specific.
While we're at it, this also simplifies the directory structure a bit.