-
Notifications
You must be signed in to change notification settings - Fork 21
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
Added Solution for Assignment 2.1 #62
base: master
Are you sure you want to change the base?
Conversation
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.
The code for CommandLineParser should not appear here. Use a separate branch for separate problem.
if process_command != "build": | ||
raise Exception(process_command + ' is not a recognized process command') | ||
return |
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.
Hmm, this "process_command" thing shouldn't be necessary, if there is only 1 allowed value for this.
with open(os.path.join("build.json")) as json_file: | ||
scripts = json.load(json_file) | ||
|
||
for script in scripts: |
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.
Lets call these actions, or build-rules, or something.
I recommend creating a separate class for an Action, and maintaining a proper graph, because it will be useful in part3 of the problem.
new_path = "" | ||
for path_segment in path: | ||
new_path += path_segment | ||
self.execute_command(new_path, current_working_directory, process_command, new_script) |
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.
You need to detect circular dependencies and show appropriate error message.
new_path += path_segment | ||
self.execute_command(new_path, current_working_directory, process_command, new_script) | ||
current_command = script['command'] | ||
os.system(current_command) |
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 vaguely satisfied part1 of the problem statement, lets do part2 now.
scripts = json.load(json_file) | ||
|
||
for script in scripts: | ||
if script['name'] == current_script: |
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 if I provide current_script = "algorithms/sort_bubble" ? The name in build.json will only be "sort_bubble", and you're not looking in the "algorithms" directory.
os.chdir(previous_location) | ||
|
||
|
||
def test_build_run(self): |
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.
Instead of creating new files in the current directory, use tempfile.TemporaryDirectory so that you can run multiple copies of the test in parallel.
|
||
def test_build_run(self): | ||
build_automation_tool_object = Build.BuildAutomationTool() | ||
build_automation_tool_object.execute_command(os.getcwd(), os.getcwd(), 'build', 'run') |
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 API is suboptimal.
The fact that you need to maintain previous_working_directory is an implementation detail, that is an internal problem for the library, and should not affect the API.
Can you do something like:
BuildAutomationTool(root=".").execute("algorithms/sort_bubble")
Added solution for Build Automation Tool along with unit tests.