Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Gremlin Base Functionality #47

Merged
merged 7 commits into from Sep 21, 2012

Conversation

Projects
None yet
2 participants
Contributor

sprjr commented Sep 11, 2012

[Edited by @aseemk: see previous discussion at issue #44.]

Hey guys,

So I believe I finished a basic, working support of the Gremlin plugin for this module. I did not update the documentation, README, or module version as I know there are other features in the works that pertain to all of those.

Included some starter tests that seem to indicate it is working, I'm sure further testing and usage will prove otherwise but I'll keep working on it. My next steps are to see if transactional & multi-line gremlin work as intended with it as well.

Let me know if you need anything else on this though!

Usage: db.execute(script, optional: params, callback)
Test: npm run gremlin

Owner

aseemk commented Sep 11, 2012

Awesome stuff! I'll take a look in depth, and we can hopefully release this, sometime this week!

@ghost ghost assigned aseemk Sep 11, 2012

@aseemk aseemk referenced this pull request Sep 11, 2012

Closed

Gremlin support #18

Owner

aseemk commented Sep 11, 2012

Hey good news, it turns out my transform code didn't handle paths properly, and in the process of fixing that I went ahead and made it recurse maps too:

0551fb2

So now you shouldn't need any special-case for Gremlin results. But let me know if you still do.

(You can rebase your branch onto the latest develop to test.)

Contributor

sprjr commented Sep 12, 2012

Awesome. Yea, I need to get a bit more thorough with the tests I have after looking at some of yours for the cypher ones with the transform function, I'll see if I can figure out this "rebase" magic you mention, get the update worked in and tested, and get it pushed back up to ya.

Owner

aseemk commented Sep 12, 2012

Try this:

# start at your latest commit
$ git checkout develop

# create a pointer to this commit as-is for backup
$ git branch backup

# assuming the remote for this thingdom repo is called upstream, fetch the latest
$ git fetch upstream

# now rebase your develop onto the latest thingdom develop
$ git rebase upstream/develop

# hopefully no conflicts. but if there are, just follow the instructions:
# - fix the conflicts, e.g. using git mergetool
# - then git rebase --continue

# if a seeming-conflict arises but there are no changed files:
# - git rebase --skip

# if at any point things are totally fubar:
# - git rebase --abort

# now if you do a git log, you should see your changes cleanly on top of the ones here
$ git log --graph --decorate --pretty=oneline --abbrev-commit

# then update your github repo (which'll update this pull request)
# assuming you call that remote origin
$ git push origin develop -f

A helpful reference:
http://git-scm.com/book/en/Git-Branching-Rebasing

And for future reference, you might prefer to do this kind of work in a feature (AKA topic) branch instead of in develop. That way you can always easily keep your develop in sync with the upstream develop, and then it's just a simple rebase of your topic branch onto your own develop.

Hope this helps!

Stephen Rivas JR added some commits Aug 22, 2012

Added functionality to execture Gremlin Scripts on the graph services…
…, was able to wire up the function and build a working starting point, am not getting callbacks to fire when results are complete
Changed the name of the gremlin function to execute after talking it …
…over with Aseem. I also commented out the results mapping

in order to get the functionality tested and working. I plan to write more tests and reapply this section in the near future.
Included a few initial Gremlin tests and verified data with a local g…
…raph. Results returned as expected. Need to rework a bit of graceful handling, but basic functionality is there
Merged in new util.transform() features from thingdom/node-neo4j-deve…
…lop, worked in

to return functionality with .execute(), tests still passing although they may need flushed
out a bit more thoroughly.
Contributor

sprjr commented Sep 14, 2012

Thanks for the rebase tips and resources, that was a first. I like the idea of using topic branches and utilizing that in future projects though - awesome stuff.

This commit passed the tests I have already written: sprjr/node-neo4j@318d68e

Owner

aseemk commented Sep 21, 2012

Sorry for the delay here, but finally getting to this, and it looks great! I'm just doing a bit of cleanup -- I'll leave comments here for future reference, but don't worry, I'm already taking care of them.

Thanks again @sprjr!

+ try
+ services = @getServices _
+ endpoint = services.gremlin or
+ services.extensions?.GremlinPlugin?['execute_script']
@aseemk

aseemk Sep 21, 2012

Owner

There is no official gremlin endpoint baked in, so simplifying this to only check the plugin endpoint.

@@ -259,6 +298,16 @@ module.exports = class GraphDatabase
actual.call @, query, params, callback
@aseemk

aseemk Sep 21, 2012

Owner

Minor note: the overload helper for query() should remain just after the query() method itself, so I'll move this up.

@@ -24,7 +24,8 @@
"clean": "rm lib/*.js",
"postinstall": "npm run build",
"profile": "_coffee test/profile",
- "test": "_coffee test"
+ "test": "_coffee test",
+ "gremlin": "_coffee test/gremlin"
@aseemk

aseemk Sep 21, 2012

Owner

I assume this was for testing during development. =) I'll go ahead remove this, but I'd really like to port our tests to Mocha, which among other things will let you grep which test cases to run!

+""", {}, _
+
+assert.ok typeof results, 'object'
+assert.ok typeof results.data.name, 'string' # dislike this because it will throw for the wrong reasons possibly
@aseemk

aseemk Sep 21, 2012

Owner

You can use results.data?.name to guard against those. =)

@aseemk

aseemk Sep 21, 2012

Owner

But in this case, you don't need to check the name's type since you're already checking its value (via strict equality from assert.equal).

@aseemk

aseemk Sep 21, 2012

Owner

Oh also, watch out -- if you're checking typeof, you want to use assert.equal, not assert.ok. (Tricky because instanceof is the opposite.) I'd love to move to expect.js or similar when moving to Mocha.

+# test: can query a single user
+results = db.execute """
+ g.v(#{user0.id})
+""", {}, _
@aseemk

aseemk Sep 21, 2012

Owner

Given that you already test no params at the end, I'll change this to test params support since it doesn't seem like any other case tests that.

+ g.v(#{user0.id}).in('gremlin_follows')
+""", {} , _
+
+# above is working, but lib should support returning instances of Node and Relationship if possible
@aseemk

aseemk Sep 21, 2012

Owner

It does now! ;)

aseemk added a commit that referenced this pull request Sep 21, 2012

aseemk added a commit that referenced this pull request Sep 21, 2012

@aseemk aseemk merged commit 318d68e into thingdom:develop Sep 21, 2012

Owner

aseemk commented Sep 21, 2012

Merged! Thanks and great work again @sprjr. =) I'll post here when this gets published to npm.

Owner

aseemk commented Sep 21, 2012

Okay, published as 0.2.17! Woot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment