Hello, > I was reading through the gremlin_python GLV code this morning, and > overall it looks like it should work pretty smoothly. Thanks for doing > this Marko, really cool work! I just wanted to comment on a few things > that popped out at me on my first reading.The major issue I see is > that it is not currently Python 2/3 compatible. This is due to two > things:
Cool! Thank you for taking your time to review the work. > 1. The way iterators are implemented in Python 2/3 is different. This > problem could be remedied by adding a method to > ``PythonGraphTraversal``, something like: > > def __next__(self): > return self.next() > > then, in the ``next`` method, changing line 113 to > ``next(self.results)``, should take care of this problem. Updated. > 2. The ``GroovyTranslator`` class checks for type ``long``, this no > longer exists in Python 3. Determining what to submit as a Long might > require some discussion, but this could be easily fixed by adding > something like: > > import sys > if sys.version_info.major > 2: > long = int > > to the top of the file. Updated. > > Other than this, there are some minor details that could be cleaned > up. Particularly: > > 1. Using ``isinstance`` instead of ``type`` to perform type checking. > This will recognize subclasses and is the most Pythonic way to do > this. Seems isinstance() isn’t a method on primitives — only objects. Thus, the code got complex. Left it with type(). > > 2. Formatting - indents, and line spacing. Typically, Python methods > are separated by a single line, and indents use four spaces. This is > really just cosmetic for readability. Uh. The problem is that the source code is auto-generated from GremlinPythonGenerator. If you want to tweak, please do so: https://github.com/apache/tinkerpop/blob/TINKERPOP-1278/gremlin-variant/src/main/groovy/org/apache/tinkerpop/gremlin/python/GremlinPythonGenerator.groovy <https://github.com/apache/tinkerpop/blob/TINKERPOP-1278/gremlin-variant/src/main/groovy/org/apache/tinkerpop/gremlin/python/GremlinPythonGenerator.groovy> > 3. CamelCase vs. underscores. I understand that to emulate Gremlin, > the traversal methods etc. should use CamelCase. But I wonder if the > helper classes (Translators) should use the Python convention of using > underscores to name methods. Python class names use camel case by > convention. I haven’t changed it. If you feel we should, please do. And yes, I think its good to keep the Gremlin step names camelCase. For Gremlin-Ruby, we will do out_e() style. > Finally, the implementation of the B class may need some work, but > we'll have to play around with it a bit to figure out how the best > approach to doing this. Yea, thats all wrong. I overdosed on the introspection and then Kuppitz was like “Why not just have it as….” (something much simpler — which I forget what it was now). > I'm sure there are more improvements, but I just wanted to get a > conversation going. I would be happy to make a PR with some of these > changes. That’d be awesome. Its in TINKERPOP-1278 branch. In gremlin-variant/test you will see PythonProcessStandardTest and PythonProcessComputerTest. Those verify that the compilation is valid for all the standard and computer tests. > Also, gremlinclient will soon support the RemoteConnection interface, > I'll send out a link to the docs once I get everything up and running. So cool. Please do review the Python RemoteConnection class as again, I just improv’d it. Thanks again David, Marko. http://markorodriguez.com