Variations of addV() and V()

[spmallette]

Ogre has both addV from GraphTraversal and add-V from GraphTraversalSource- It would be nice if the functions didn't have to be named that way and could be more consistent.

Equally irritating is V and midV where the latter is use for a mid-traversal V(). Would be nice if midV could just be V somehow or another.

[shooit]

I just ran into this. Perhaps the solution is to use a protocol for the overlapping functions between GraphTraversal and GraphTraversalSource and extend accordingly. Would be happy to spend some cycles on it if amenable.

[spmallette]

That would be great @shooit - i sorta just hacked in those weird named methods to get the API coverage complete. It would be really nice to see a cleaner approach. thanks

[shooit]

No problem. I see three ways to clean up the API. They all have their pluses and minuses. I think I lean towards the third approach. The code provided is a sketch and I haven't tested it thouroughly but I would be happy to add coverage on the preferred implementation.

1. Remove the type hints on the affected functions: addE, add-E, addV, add-V, V, midV and pick one version to keep. This would change any errors from calling the function on an object that does not have the underlying Java method from a ClassCastException to an IllegalArgumentExeception. A reflection warning would also be logged because of the :global-vars setting in project.clj.

2. Add a CommonTraversal protocol. This requires having both a "proto" and a normal version of the V function because protocols don't support variable args. Errors caused by calling the function on an unsupported type will be IllegalArgumentException but the compiler puts type hints on the first arg.

   
   (defprotocol CommonTraversal
   (add-V [g] [g label])
   (add-E [g label])
   (proto-V [g ids]))

(extend-protocol CommonTraversal GraphTraversal (add-V ([g] (.addV g)) ([g label] (.addV g ^String (util/cast-param label)))) (add-E ([g label] (.addE g ^String (util/cast-param label)))) (proto-V [g ids] (.V g (into-array ids)))

GraphTraversalSource (add-V ([g] (.addV g)) ([g label] (.addV g ^String (util/cast-param label)))) (add-E ([g label] (.addE g ^String (util/cast-param label)))) (proto-V [g ids] (.V g (into-array ids))))

(defn V [g & ids] (proto-V g ids))

3. Add Multimethods for the affected functions. Type hints need to be added, but will only be used by the compiler to avoid reflection as any errors coming from calling the function on an unsupported type will be due to not having a `defmethod` and will be `IllegalArgumentException`.
``` clojure
(defmulti add-V (fn
                  ([g _] (class g))
                  ([g] (class g))))

(defmethod add-V GraphTraversal
  ([^GraphTraversal g] (.addV g))
  ([^GraphTraversal g label] (.addV g ^String (util/cast-param label))))

(defmethod add-V GraphTraversalSource
  ([^GraphTraversalSource g] (.addV g))
  ([^GraphTraversalSource g label] (.addV g ^String (util/cast-param label))))

(defmulti add-E (fn [g _] (class g)))

(defmethod add-E GraphTraversal
  ([^GraphTraversal g label] (.addE g ^String (util/cast-param label))))

(defmethod add-E GraphTraversalSource
  ([^GraphTraversalSource g label] (.addE g ^String (util/cast-param label))))

(defmulti V (fn [g & _] (class g)))

(defmethod V GraphTraversal
  [^GraphTraversal g & ids]
  (.V g (into-array ids)))

(defmethod V GraphTraversalSource
  [^GraphTraversalSource g & ids]
  (.V g (into-array ids)))

[shooit]

One more thought... w.r.t. not dropping support for any of the current functions, we could add all of the names to the implementations, e.g. addV, midV. I think some guy named Rich thinks that's a good idea ;)

[spmallette]

Is there any approach you recommend? I think I like the multi-method approach, but i'm not sure i fully understand the ups/downs for each of the options you provided.

[shooit]

I like the multi-method approach best also. It would result in no changes to the public API and makes use of typehints in keeping with the rest of the code. I'll work on getting a PR up for it.

[shooit]

I have a branch ready for review but don't have permissions to push it to GitHub. All current unit tests are passing. I would like to expand the test coverage to these functions over both types but I don't understand how the testing suite is working across the Clojure and Java code.

[spmallette]

I have a branch ready for review but don't have permissions to push it to GitHub.

did you not fork the repository so as to issue a pull request?

[michaelklishin]

Stephen is right, you need to push it to your fork and submit a pull request. Or at least tell us what repo and branch to pull from.

[shooit]

Got it. Just made the pull request. Thanks!

[spmallette]

fixed on #99