Common api for GraphTraversal and GraphTraversalSource

[shooit]

This commit moves the following functions into multimethods that work for both GraphTraversal and GraphTraversalSource objects

• add-V
• addV
• add-E
• addE
• V
• midV

Both versions of the fuctions are maintained to preserve backwards compatibility of the API.

[spmallette]

Travis failed, but i'm not sure the error is relevant to your change. It builds for me locally. I just re-triggered the build so we'll see if it goes green or if there is something to investigate there. I've not had a chance yet to review the code specifically yet. For me, it may need to wait until after the holiday. Thanks for your work and patience on this @shooit

[shooit]

Hey, just circling back to this. I see that it fails in Travis, but it also builds for me locally. I didn't quite understand the test suite, but I'd be happy to dive in on that if anyone has time to explain how its working. I am more used to midje/clojure.test.

[spmallette]

The error message on travis is: "Tried to use insecure HTTP repository without TLS." There is an entry in the lein faq on that:

https://github.com/technomancy/leiningen/blob/master/doc/FAQ.md

Not sure what that means though - does it just mean we need to change the repos in the project.clj to use "https"?

[shooit]

It might be as simple as that. I've had to do that for other projects. I updated the project.clj to use https but I cant figure out what dependencies are getting pulled from the oss.sonatype.org repos to test the change. Is Travis looking there instead of maven central?

[shooit]

Looks like Travis is passing now!

[spmallette]

travis looks happy now - i guess that fixed things. here's a few notes about testing to help you get familiar with how things work.

Ogre is a form of GLV. As clojure is a JVM language it simply wraps the Java classes of TinkerPop and focuses on the Traversal API - this is quite similar to how gremlin-scala works. To test Ogre, we run Ogre against the TinkerPop language test suite (which is typically implemented by graph database providers). To make this all work, there is a fair bit of annoying boilerplate code that glues all that together to work with the clojure testing framework and lein.

Basically, this is the only test that is run:

https://github.com/clojurewerkz/ogre/blob/6de9cb009fa067d9a5b49854cf9f88afc658bc12/test/java/org/clojurewerkz/ogre/gremlin/process/OgreTinkerGraphProcessStandardTest.java

it does some TinkerPop magic that executes the body of tests listed in this suite:

https://github.com/clojurewerkz/ogre/blob/6de9cb009fa067d9a5b49854cf9f88afc658bc12/test/java/org/clojurewerkz/ogre/gremlin/process/OgreProcessStandardSuite.java

Those are all "process" (i.e. Gremlin language) test from TinkerPop essentially. That leads us to this miserable class:

https://github.com/clojurewerkz/ogre/blob/6de9cb009fa067d9a5b49854cf9f88afc658bc12/test/java/org/clojurewerkz/ogre/gremlin/process/OgreTinkerPopCheck.java

It basically represents a proxy to all the TinkerPop tests which are implemented in clojure over here:

https://github.com/clojurewerkz/ogre/tree/6de9cb009fa067d9a5b49854cf9f88afc658bc12/test/clojure/clojurewerkz/ogre/suite

The clojure tests merely return the clojure representation of a GraphTraversal that the TinkerPop test expects. Then that GraphTraversal is asserted back in the TinkerPop tests themselves. As the TinkerPop test suite is fairly complete, we don't really write tests outside of those.

So those are the mechanics...which then leads to how you might write tests that aren't in the TinkerPop test suite. With your PR we now have this situation where we probably would want to test both the old methods of add-V() and your new method of addV(). Not sure how we do that exactly. I kinda think you should use the "preferred" addV() in the TinkerPop test suite:

https://github.com/clojurewerkz/ogre/blob/6de9cb009fa067d9a5b49854cf9f88afc658bc12/test/clojure/clojurewerkz/ogre/suite/add_vertex_test.clj

and then perhaps we start a new separate set of pure clojure tests (i.e nothing to do with the TinkerPop test suite) that will test the old methods directly. does any of that make sense?

[spmallette]

@shooit this is a really nice pull request. my understanding of multimethods is much better having taken the time to understand the usage in this context - so i have to thank you for that as well. did you want to make an attempt at writing additional tests for this based on what I wrote in the previous comment? if not, just let me know and I can try to figure it out upon merging. thanks!

[shooit]

@spmallette thanks! I got pulled into other tasks at work this week but I was just about to come back to the project that prompted this PR. I looked over the description of the test framework and I think I know what to do. Adding pure Clojure tests for anything that still lacks coverage should be easy. I will tackle it ASAP unless you are eager to do it now.

In terms of preferred functions, I think add-V > addV, add-E > addE, and V > midV. I'd go as far as to suggest that a future release have add-v, add-e, v, and e per this comment in the GLV docs:

If camelCase is not an accepted method naming convention in the host language, then the host language’s convention can be used instead. For instance, in a Gremlin-Ruby implementation, outE("created") may be out_e("created").

[spmallette]

I'm in no rush - so feel free to get to the tests when you're free to do so. I'm also open to discussions regarding other API changes if we'd like the naming more clojure-like. V and E as capitals just has such strong meaning in Gremlin that I thought keeping that here was good, but I'm not bent on keeping it that way if there is stronger opinion to change.

[shooit]

@spmallette I updated all of the test suite to use the new multimethods. I also realized that we could just def the secondary name of the functions to the primary so there is less code repetition. In updating the tests I learned more about what anon.clj was doing and made similar updates to that as in core.clj so that both function names will be accepted. I haven't added extra tests for the secondary function names since they are simply def'd to the primary. Happy to add if you feel it is necessary.

[spmallette]

Nice - i had meant to ask about anonymous traversals. I think we can avoid the extra tests. On a future release when we allow for some breaking changes, we can just clean up and kill the old method names. I'll give this one final review and look to get this merged later today. I'll also give a review of the Ogre documentation and see what (if anything) should be updated.

[spmallette]

Thanks again for your efforts (and patience) on this PR @shooit . It ended up quite nicely.

[shooit]

You're welcome! Very happy to contribute. Is there a timeline on getting a release cut that includes these changes?

[spmallette]

no - didn't have timeline in mind though i'm trying to get ogre working with TinkerPop 3.3.1 in my spare time. i assume we'd look to release something once that's done. i'd guess it could happen in the couple of weeks assuming there are no other problems.

i noticed that now that we do add-V the use of outE, inV(), etc all look weird. starting to wonder if those should all switch over to include the dash.

[shooit]

Got it. I am definitely in favor of moving toward dashifying the function names. It might be worth waiting until you are open to making a breaking change in the API to avoid having a bunch of duplicated functions. Maybe that goes along with the TinkerPop version update?

[spmallette]

TinkerPop allows breaks in API in the y of x.y.z, so ideally Ogre would wait until a 3.4.0 release of TinkerPop