Uploaded image for project: 'TinkerPop'
  1. TinkerPop
  2. TINKERPOP-1418

CoreTraversalTests depend on missing functionality

Details

    • Improvement
    • Status: Closed
    • Minor
    • Resolution: Done
    • 3.2.1
    • 3.1.4, 3.2.2
    • documentation
    • None

    Description

      The shouldLoadVerticesViaVertices and shouldLoadEdgesViaEdges tests in CoreTraversalTests.java appear to depend on unspecified functionality.

      The comment for the .vertices method in the Graph structure class does not specify that it should accept Vertex objects (except in one odd place) - as an implementor it reads as if it's intended to be called only with identifiers.

      When these two tests were added some functionality was included in GraphStep to handle mapping Vertex objects to their IDs; this functionality was soon removed, however, and added to TinkerGraph. Incidentally, Titan also implements the same functionality.

      Is Graph.vertices(Object...) intended to gracefully handle an array of Vertex elements as well as an array of Vertex identifiers? If so, then it appears that the documentation in Graph.java needs some reworking. It'd also perhaps be unfortunate, stylistically, to add a third behavior to that method. If it's not intended to support that call, then it seems that these two methods (and perhaps others?) should be removed from the test suite.

      Attachments

        Activity

          The intention is to allow:

          Vertex v = 
          graph.vertices(v)
          

          Documentation could be more clear on that

          spmallette Stephen Mallette added a comment - The intention is to allow: Vertex v = graph.vertices(v) Documentation could be more clear on that

          Alrighty. There's a dozen or so references in the comments for Graph.vertices and Graph.edges that pretty clearly point to element ids.

          Out of curiosity, why was this behavior removed from GraphStep and pushed down to the implementor?

          banjiewen Benjamin Anderson added a comment - Alrighty. There's a dozen or so references in the comments for Graph.vertices and Graph.edges that pretty clearly point to element ids. Out of curiosity, why was this behavior removed from GraphStep and pushed down to the implementor?

          i stared at the commit you referenced for a while, but i don't seem to remember why it was necessary. i think it's one of those things where there was a good reason, but it's just been forgotten.....dah

          spmallette Stephen Mallette added a comment - i stared at the commit you referenced for a while, but i don't seem to remember why it was necessary. i think it's one of those things where there was a good reason, but it's just been forgotten.....dah

          Made some minor reclassifications on the ticket given the previous comments.

          spmallette Stephen Mallette added a comment - Made some minor reclassifications on the ticket given the previous comments.
          spmallette Stephen Mallette added a comment - Improved javadoc and resolved via CTR on https://github.com/apache/tinkerpop/commit/0fefa12cd5d5ff80da254242db7c5ef3ae2d69cf

          People

            spmallette Stephen Mallette
            banjiewen Benjamin Anderson
            Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved: