David, it would also be great to get your feedback on the packaging/deployment approach we have so far. I have some basic figured out for deployment to pypi over maven via twine, but it could use an experts eye. You probably don't need to check that part out now as you are already have some other stuff to look at, but I just wanted to mention that so that it was in your mind for later. Thanks for your help on this.
On Wed, Jun 15, 2016 at 3:02 PM, Marko Rodriguez <[email protected]> wrote: > 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
