Adding GitImplementation of Ground and tests by avinash-arjavalingam · Pull Request #4 · ground-context/client

avinash-arjavalingam · 2018-03-06T21:26:12Z

Added the ground.py file, originally from https://github.com/ucbrise/jarvis, now with all datatypes implemented and all the GitImplementation methods completed, as well as adding unimplemented methods for the GroundAPI abstract class, and making some minor fixes along the way. Also, the testing file test_ground.py was added, which can be run from the command line with python 2 as 'python test_ground.py'. It contains comprehensive testing for the datatypes and GitImplementation.

…ound.py

vsreekanti

All of these files shouldn't be in the home directory. They should be organized under the python directory in this repo.

I think we need to chat about this a little more. I don't see anything in the code that uses the commit method or the git library imported at the top. Everything is being stored in memory, which defeats the purpose of using git. Let me know if I'm misreading how this works.

vsreekanti · 2018-03-07T19:56:09Z

ground.py

+from shutil import copyfile
+
+
+class Node:


All these classes are already implemented in python/ground/common/model. We shouldn't duplicate them.

vsreekanti · 2018-03-07T20:18:45Z

ground.py

+        if (nodeId in self.graph.nodes):
+            node = self.graph.nodes[nodeId]
+        nodeVersion = NodeVersion(node, reference, referenceParameters, tags, structureVersionId, parentIds)
+        # else:


This is Jarvis specific stuff. We should remove this.

vsreekanti · 2018-03-07T20:21:20Z

ground.py

+
+        ### EDGES ###
+
+    def createEdge(self, sourceKey, fromNodeId, toNodeId, name="null", tags=None):


Maybe I'm missing something, but none of these methods are actually using git commands. We're returning heap objects which won't persist across multiple API calls.

vsreekanti · 2018-03-07T20:23:03Z

ground.py

+    def getLineageGraphVersion(self, lineageGraphVersionId):
+        return self.graph.lineageGraphVersions[lineageGraphVersionId]
+
+    def commit(self):


I also don't see this commit method used anywhere.

…on files

vsreekanti

Mostly looks good. Most of my questions are about code cleanup and style. One more quick revision should do the trick!

vsreekanti · 2018-03-20T17:48:17Z

python/ground_git.py

+from shutil import copyfile
+# noinspection PyUnresolvedReferences
+from ground.common.model.core.node import Node
+# noinspection PyUnresolvedReferences


What are these comments?

Those are just for my IDE, PyCharm, so it doesn't stop a run for errors. For whatever reason, PyCharm doesn't recognize the importing of local packages, so it will show an error that is not really there. I can remove those if that is preferable

Yeah, please remove them from the code.

Bump on this.

vsreekanti · 2018-03-20T17:50:08Z

python/ground_git.py

+"""
+Abstract class: do not instantiate
+"""
+


Nit: Extra whitespace.

vsreekanti · 2018-03-20T17:51:12Z

python/ground_git.py

+
+class GitImplementation(GroundAPI):
+    def __init__(self):
+        #self.nodes = {}


If this is unnecessary, we should remove.

vsreekanti · 2018-03-20T17:54:14Z

python/ground_git.py

+    def _gen_id(self):
+        with open(self.path + 'ids.json', 'r') as f:
+            ids = json.loads(f.read())
+        newid = len(ids)


Why can't we just store an integer?

vsreekanti · 2018-03-20T17:54:46Z

python/ground_git.py

+            f2.write(json.dumps(ids))
+        return newid
+
+    def _write_files(self, id, body):


Why not reuse this fn in _gen_id?

(And _read_files below.)

vsreekanti · 2018-03-20T17:56:50Z

python/ground_git.py

+        return str(output, 'UTF-8')
+
+    def _check_init(self):
+        if(not self.initialized):


What happens if I created a repo a long time ago and am reusing it now? How do I know that it's been initialized?

the .init() method will automatically check for previous initialization and will call init again which is safe but will not replace the repo and your existing work. The ids will carry on with the highest id and continue, not replacing older files. However, this implementation does not handle the case where the folder has been previously manually created, then initialized to some other repo with previous commits that have no relation to the ground objects, it will just continue to commit to that initial repo.

That's fine for now. Let's add a README.md to the directory with documentation like this.

vsreekanti · 2018-03-20T17:57:28Z

python/ground_git.py

+            body["toNodeId"] = toNodeId
+            edge = Edge(body)
+            edgeId = edge.get_id()
+            #self.edges[sourceKey] = edge


Nit: Cleanup commented out code.

vsreekanti · 2018-03-20T18:00:14Z

python/ground_git.py

+    def createNode(self, sourceKey, name="null", tags=None):
+        self._check_init()
+        if not self._find_file(sourceKey, Node.__name__):
+            body = self._create_item(Node.__name__, sourceKey, name, tags)


We seem to be reusing the class names a lot here. Not necessarily a bad thing, but can we think of a more efficient solution?

vsreekanti · 2018-03-20T18:01:11Z

python/ground_git.py

+        self._check_init()
+        return self._read_version(lineageGraphVersionId, LineageGraphVersion.__name__)
+
+    """


Same nit about commented code.

vsreekanti · 2018-03-20T18:01:34Z

python/ground_git.py

+        os.chdir('../' + str(int(prevDir) + 1))
+    """
+
+class GroundImplementation(GroundAPI):


What is this?

Holdover from the file in jarvis, unnecessary and will delete

…riables for class names, and fixed the latest id method

Add ground.py to implement GitImplementation and testing file test_gr…

833a8de

…ound.py

vsreekanti requested changes Mar 7, 2018

View reviewed changes

Correct errors, add commit for every creation, save the objects in js…

30c27e4

…on files

vsreekanti requested changes Mar 20, 2018

View reviewed changes

aviarjavalingam added 2 commits April 13, 2018 22:49

Removed unnecessary classes, comments with old code, and noinspect

7b73d24

Made the requested changes, including adding README.md, have class va…

b0cd278

…riables for class names, and fixed the latest id method

vsreekanti approved these changes Apr 16, 2018

View reviewed changes


		### EDGES ###

		def createEdge(self, sourceKey, fromNodeId, toNodeId, name="null", tags=None):

Conversation

avinash-arjavalingam commented Mar 6, 2018

Uh oh!

vsreekanti left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vsreekanti left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants