Solution to assignment-4 (assignment-2.1 on hold) #58
Conversation
|
please review the pull request |
solutions/assignment-2/test.py
Outdated
|
|
||
| cmd = r'python build.py --root=~\tmp\ parent' | ||
| output = subprocess.check_output(cmd, shell=True) | ||
| self.assertEqual(output, b'Error: Undefined dependencies found "B".\r\n') |
There was a problem hiding this comment.
The message can explain which build rule tried to depend on it.
solutions/assignment-2/test.py
Outdated
| C | ||
| / \ | ||
| D E | ||
| """ |
solutions/assignment-2/test.py
Outdated
| @@ -0,0 +1,632 @@ | |||
| import os | |||
There was a problem hiding this comment.
Btw, this file is too large. You can split it down into smaller ones.
solutions/assignment-2/test.py
Outdated
| std::cout<<"sort_merge_modified"; | ||
| } | ||
| """) | ||
| sleep(10) |
There was a problem hiding this comment.
Tests with sleep statements are non-ideal, since they make you wait.
Can you modify your code so that something like this is possible?
# assuming something like:
def watch():
with True:
self.check_and_maybe_execute()
And then you call check_and_maybe_execute from this file, eliminating any wait.
solutions/assignment-2/test.py
Outdated
| self.delete_file_structure(root='tmp') | ||
|
|
||
| def test_ignore_files(self): | ||
| FILE_STRUCTURE = { |
There was a problem hiding this comment.
Since this has been repeated a few times, make it a class level variable and share it? You can use it like a template, and add modifications on top of a deepcopy.
| [ | ||
| { | ||
| "name": "runA", | ||
| "deps": ["dirB\\runB"], |
There was a problem hiding this comment.
Earlier you mentioned that "dirB\\runB" is quite confusing and it does not tell whether it is an absolute path or relative path.
Here, I have assumed that all the entries which come in the "deps" is a relative path, and \\ is used to handle the escape character \, since I am using Windows os, separators for os.path is \
Any changes to "dirB\\runB" like ".\dirB\\runB" or "\\dirB\runB" was difficult because entries in "deps" should match with some "name".
There was a problem hiding this comment.
Looking good!
- There are lots of different things going on in you build.py file. You should break it down into pieces: one file for the classes for rules, another file for executing them, another to watch for changes.
- Don't separate your file structure from your tests, since they're conceptually linked. As a reader, it is ideal if all the information relevant to a single test is in one location. Feel free to separate the tests across multiple files to keep them easily readable and use a test_utils.py module to share code where needed.
- I updated the problem statement to add another requirement: parallel execution of rules without multithreading. Lets do that too :)
solutions/assignment-2/build.py
Outdated
| if os.path.getmtime(self.filename) != self.last_modified: | ||
| self.last_modified = os.path.getmtime(self.filename) |
There was a problem hiding this comment.
Call os.path.getmtime only once, since it is unnecessary duplication computation.
solutions/assignment-2/build.py
Outdated
| if 'deps' in properties: | ||
| self.dependent_rulenames = properties['deps'] |
There was a problem hiding this comment.
self.dependent_rulenames = properties.get('deps', [])
solutions/assignment-2/build.py
Outdated
| parser.add_argument('--watch', action='store_true', help='Status to build again on file modification') | ||
| parser.add_argument('--poll', type=float, default=1, help='Time interval for checking the file modification status') | ||
| parser.add_argument('--ignore', nargs='*', type=str, help='List of files not to be watched') | ||
| return vars(parser.parse_args(arguments)) |
There was a problem hiding this comment.
What is the benefit of using vars here?
solutions/assignment-2/build.py
Outdated
| self.build_rules: Dict[str, Rule] = dict() | ||
| self.build_files: Dict[str, File] = dict() | ||
| self.build_arguments: Dict[str, Any] = self._parse(arguments) | ||
| self.build_arguments['root'] = os.path.realpath(self.build_arguments['root']) |
There was a problem hiding this comment.
Instead of doing this in a separate phase, you can try:
parser.add_argument('--root', type=lambda path: os.path.realpath(path), ...
solutions/assignment-2/build.py
Outdated
| class File(object): | ||
| """docstring for File""" | ||
|
|
||
| def __init__(self, filename: str, is_BUILD_FILE: bool=False): |
There was a problem hiding this comment.
Confusing use of capital letters here ... why not just use "is_build_file"?
solutions/assignment-2/graph.py
Outdated
| processed: Dict[Any, int] = {key: -1 for key in self.adjacency_list} | ||
| dfs_list: List[Any] = list() | ||
| self._dfs(source, processed, dfs_list) | ||
| dfs_list.reverse() |
There was a problem hiding this comment.
You reversed this list here, and also in the other file where you called this method. Unnecessary work?
solutions/assignment-2/build.py
Outdated
| supporter_list: Dict[str, List[str]] = {key: list() for key in self.build_rules} | ||
| for file_name in self.build_files: | ||
| if not self.build_files[file_name].is_BUILD_FILE: | ||
| supporter_list[file_name] = list() |
There was a problem hiding this comment.
Wait, so the keys of supporter_list include both rule names and file names? Mixing things like that is probably not a good idea, and it looks like this only matters in the first step of your traversal (filename -> rule name), so you can do that manually.
solutions/assignment-2/build.py
Outdated
| raise BuildAutomationError(f'Invalid rule "{rulename}" provided') | ||
| dependent_rulenames = self.get_dependent_rulenames_for_executing_rule(rulename) | ||
| for dependent_rulename in dependent_rulenames: | ||
| output += self.build_rules[dependent_rulename].execute() |
There was a problem hiding this comment.
See Part2 of updated problem statement.
solutions/assignment-2/build.py
Outdated
| def check_and_maybe_execute(self) -> str: | ||
| output = '' | ||
| for filename in self.build_files: | ||
| if filename not in self.ignored_files: |
There was a problem hiding this comment.
Return/continue/break early to reduce indenting.
if filename in self.ignored_files:
continue
solutions/assignment-2/build.py
Outdated
| def execute(self) -> str: | ||
| process = subprocess.Popen(self.command.split(), stdout=subprocess.PIPE, shell=True) | ||
| (out, err) = process.communicate() | ||
| return out.decode() |
There was a problem hiding this comment.
Hmm, I don't think that the stdout of the process is a reliable indicator of success. I'd recommend using return value.
Hemant-Chowdhury
changed the title
solutions/assignment-4.1/Object.py
Outdated
| # pass | ||
|
|
||
|
|
||
| class commit(object): |
There was a problem hiding this comment.
solutions/assignment-4.1/Object.py
Outdated
| with open(os.path.join(vcs_files.OBJECTS, self.get_hash()), 'w') as commit_file: | ||
| commit_file.write(self.get_content()) | ||
|
|
||
| def set_hash(self, hash: str) -> None: |
There was a problem hiding this comment.
This method does everything except what the name says.
solutions/assignment-4.1/test.py
Outdated
| os.remove(filename) | ||
|
|
||
| def setUp(self): | ||
| test_dir = 'test_dir' |
There was a problem hiding this comment.
solutions/assignment-4.1/test.py
Outdated
| os.chdir(test_dir) | ||
|
|
||
| def test_init(self): | ||
| result = Init.init() |
There was a problem hiding this comment.
Why not use your vcs class here?
solutions/assignment-4.1/Init.py
Outdated
| self.status = 'Initializing vcs in this directory' | ||
| self.check_and_create_files() | ||
|
|
||
| def check_and_create_files(self): |
There was a problem hiding this comment.
This code works in case an incomplete set of those folders/files already exists.
I'm not sure if that's a good idea. Ie - if you find something partial, I would imagine it is safer to raise an exception, and ask the user to delete the things.
solutions/assignment-4.1/Commit.py
Outdated
| return | ||
| commit_object = Object.commit() | ||
| commit_object.set_parent(Object.get_HEAD()) | ||
| tree_object = self._save_object(os.getcwd()) |
There was a problem hiding this comment.
What if the repo contains multiple nested folders?
Will the commit only capture the current one?
solutions/assignment-4.1/Diff.py
Outdated
| with open(old_file, 'r') as read_old_file: | ||
| self.old_file_lines = read_old_file.read().split('\n') | ||
|
|
||
| def _lcs(self): |
There was a problem hiding this comment.
This must have been fun to implement :)
solutions/assignment-4.1/test.py
Outdated
| import unittest | ||
|
|
||
|
|
||
| class TestVCS(unittest.TestCase): |
There was a problem hiding this comment.
This is doing too many things IMO.
For the sake of readability, I would recommend multiple smaller test files, so that you can test your utility methods like LCS specifically too, not just the commands.
Please review the pull request.
Commands used in the assignment are compatible with the windows operating system.
The code is taking 3-5s to execute test.py