Thanks! I'll submit my PR in the morning. ________________________________________ From: Marko Rodriguez <[email protected]> Sent: June 15, 2016 6:13 PM To: [email protected] Subject: Re: gremlin_python GLV
Hi, That is a really good idea. Want to do a PR to the branch or do you want me to just take your notes/gist and make it happen? Marko. http://markorodriguez.com > On Jun 15, 2016, at 3:55 PM, Leifur Halldor Asgeirsson > <[email protected]> wrote: > > Hi all, > I have a couple of suggestions for the GLV. > > Firstly, it would be useful to be able to inject arbitrary expressions, such > as static constructors. For example, in an application that I'm currently > working on, I use Titan's Geoshape property type. I would like to be able to > call one of the static constructors on the Geoshape class in a script, > passing it bound parameters. Something like > > "g.V().has('location', geoWithin(Geoshape.circle(lat, lon, radius)))" > with bound parameters {'lat': 45, 'lon': 45, 'radius': '10'} > > I took an initial stab at this today, and this is what I came up with: > https://gist.github.com/leifurhauks/5a843379183123dbed1134a92691a335 > > With those changes, I can use that static constructor in a traversal as > follows: > >>>> translator = GroovyTranslator('g') >>>> g = PythonGraphTraversalSource(translator) >>>> t = g.V().has('location', RawExpression('Geoshape.point(', B('lat', 45), >>>> ', ', B('lon', 45), ')')) >>>> str(t) > 'g.V().has("location", Geoshape.point(lat, lon))' >>>> t.bindings > {'lon': 45, 'lat': 45} > > That raw expression isn't very readable, but a simple helper class can fix > that: > > class Geoshape(object): > @staticmethod > def point(latitude, longitude, symbols=('lat', 'lon')): > return RawExpression( > 'Geoshape.point(', B(symbols[0], latitude), ', ', B(symbols[1], > longitude), ')') > > Now I can rewrite the previous traversal to be much clearer: > > t = g.V().has('location', Geoshape.point(45, 45)) > > > If this seems like a reasonable approach, I would be happy to submit a PR. > > > I have one other suggestion, but this one is tiny. Because most of the steps > on PythonGraphTraversal have the same implementation, it would be possible to > specify that implementation once in a function factory, like this: > > def simple_step(name): > def step_method(self, *args): > self.translator.addStep(self, name, *args) > for arg in args: > if type(arg) is B: > self.bindings[arg.symbol] = arg.value > return self > return step_method > > Then, on PythonGraphTraversal, all the step methods that use that > implementation could be declared like this: > > class PythonGraphTraversal(object): > def __init__(self, translator, remote_connection=None): > # elided ... > # top methods elided > has = simple_step('has') > hasId = simple_step('hasId') > hasKey = smple_step('hasKey') > # and so on... > > > ________________________________________ > From: David Brown <[email protected]> > Sent: June 15, 2016 3:23 PM > To: [email protected] > Subject: Re: gremlin_python GLV > > 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
