Ok Stephen I will. I can't claim to be an expert (especially when it comes to the Java ecosystem), but I will definitely take a look.
Also, Marko, Python doesn't really have primitives as such. The built in function isinstance should work for everything in gremlin_python e.g. ``isinstance(var, bool)``. I can take a look at this when I make a PR if you'd like, but it really isn't a huge deal anyway. On Wed, Jun 15, 2016 at 3:16 PM, Stephen Mallette <[email protected]> wrote: > 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 -- David M. Brown R.A. CulturePlex Lab, Western University
