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
