Skip to content
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

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

pranjalrai
Copy link

Added solution for Build Automation Tool along with unit tests.

Copy link
Owner

@kaustubh-karkare kaustubh-karkare left a 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.

Comment on lines +10 to +12
if process_command != "build":
raise Exception(process_command + ' is not a recognized process command')
return
Copy link
Owner

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:
Copy link
Owner

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)
Copy link
Owner

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)
Copy link
Owner

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:
Copy link
Owner

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):
Copy link
Owner

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')
Copy link
Owner

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")

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants