[GitHub] [tinkerpop] spmallette commented on pull request #1545: Orderability Semantics (final)

[GitBox <gi...@apache.org>]

spmallette commented on pull request #1545:
URL: https://github.com/apache/tinkerpop/pull/1545#issuecomment-1013081630


   This is a glorious looking PR @mikepersonick (and @divijvaidya is no slouch on the code reviews - watch out @nastra you have competition in the category for "Best Code Reviewer"! 😄 )
   
   Tests look really good. I think the upgrade docs could use a bit of sample code to demonstrate all you wrote there:
   
   ```text
   gremlin> g.V().values().order()  // 3.5.x
   java.lang.String cannot be cast to java.lang.Integer
   Type ':help' or ':h' for help.
   Display stack trace? [yN]n
   gremlin> g.V().values().order()  // 3.6.0
   <whatever the output is>
   ```
   
   As an administrative note, a rebase to cleanup the commit history and to get us to one commit would be cool (unless you feel some commits are better here individually for some reason).
   
   The changes to Graph.java around removing the method scope isn't a big deal, but it is a bit unlike all our other interface definitions. i've often thought about switching our style there to how you have it as every IDE nags about it. maybe we should just go all in and do that code reformat at some point.
   
   Other than that VOTE +1 - let's see if we can find a final committer to ok this so we can get this merged today.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org