[GitHub] [tinkerpop] divijvaidya commented on a change in pull request #1545: Orderability Semantics (final)

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

divijvaidya commented on a change in pull request #1545:
URL: https://github.com/apache/tinkerpop/pull/1545#discussion_r782906051



##########
File path: gremlin-javascript/src/main/javascript/gremlin-javascript/test/cucumber/feature-steps.js
##########
@@ -298,6 +305,17 @@ function toPath(value) {
   return new Path(new Array(0), parts.map(x => parseValue.call(this, x)));
 }
 
+/*
+ * TODO Implement me.

Review comment:
       Please create a JIRA and add here

##########
File path: gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/driver/remote/AbstractRemoteGraphProvider.java
##########
@@ -206,6 +206,10 @@
         test = "org.apache.tinkerpop.gremlin.process.traversal.step.map.UnfoldTest",
         method = "g_V_valueMap_unfold_mapXkeyX",
         reason = "Tests that include lambdas are not supported by the test suite for remotes")
+@Graph.OptOut(
+        test = "org.apache.tinkerpop.gremlin.process.traversal.step.OrderabilityTest",
+        method = "g_inject_order_with_unknown_type",
+        reason = "Tests that inject a generic Java Object are not supported by the test suite for remotes")

Review comment:
       Nice. We can use same reason for at rest of the places.

##########
File path: gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Gherkin/GherkinTestRunner.cs
##########
@@ -85,6 +85,8 @@ public class GherkinTestRunner
                     "g_V_hasXperson_name_markoX_bothXknowsX_groupCount_byXvaluesXnameX_foldX",
                     IgnoreReason.ArrayKeysInMapNotAssertingInGherkin
                 },
+                {"g_V_properties_order", IgnoreReason.NoReason},

Review comment:
       Please add a reason for Ignore here. A comment should be sufficient.

##########
File path: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/Graph.java
##########
@@ -61,13 +61,13 @@
     /**
      * Configuration key used by {@link GraphFactory}} to determine which graph to instantiate.
      */
-    public static final String GRAPH = "gremlin.graph";
+    String GRAPH = "gremlin.graph";
 
     /**
      * This should only be used by providers to create keys, labels, etc. in a namespace safe from users.
      * Users are not allowed to generate property keys, step labels, etc. that are key'd "hidden".
      */
-    public static class Hidden {
+    class Hidden {

Review comment:
       Restricting the scope is a backward incompatible change since vendors might be extending this class. Please revert back to public scope.
   
   Same for other changes in this file.

##########
File path: tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/io/graphson/AbstractTinkerGraphGraphSONTranslatorProvider.java
##########
@@ -242,12 +242,24 @@ public GraphTraversalSource traversal(final Graph graph) {
         return g.withStrategies(new TranslationStrategy(g, new GraphSONTranslator<>(JavaTranslator.of(g), version), true));
     }
 
+    @Graph.OptOut(
+            test = "org.apache.tinkerpop.gremlin.process.traversal.step.OrderabilityTest",
+            method = "g_inject_order",
+            reason = "GraphSONv2 does not properly round trip Maps and Sets")

Review comment:
       This might not be important for 3.6.x since GraphSonV3 is supposed to be used here but we still haven't deprecated graphsonv2 hence, it requires some attention. Please create a JIRA for this. We will have to investigate what's wrong with graphsonv2 and document the shortcoming.

##########
File path: gremlin-groovy/src/test/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GroovyTranslatorProvider.java
##########
@@ -215,6 +215,10 @@
         test = "org.apache.tinkerpop.gremlin.process.traversal.step.map.WriteTest",
         method = "*",
         reason = "read and write tests don't translate locally well because of calling iterate() inside read()/write() add a none()")
+@Graph.OptOut(
+        test = "org.apache.tinkerpop.gremlin.process.traversal.step.OrderabilityTest",
+        method = "g_inject_order_with_unknown_type",
+        reason = "Cannot round trip a generic Java Object")

Review comment:
       Maybe update the reason here to say something about how this doesn't work for remote. To a layman code reader, this leaves a cliffhanger about why cannot be roundtrip generic objects here.




-- 
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