TINKERPOP-1920 Fixed P.within/without() handling for collections by spmallette · Pull Request #817 · apache/tinkerpop

spmallette · 2018-03-15T16:32:51Z

https://issues.apache.org/jira/browse/TINKERPOP-1920

Made the logic the same as what we have in Java currently for P.within() and P.without() for handling single collection values. I pretty much just made this work so open to input from C# experts for a nicer handling of P, but for now in terms of fixing the problem:

Builds with mvn clean install -pl :gremlin-dotnet,:gremlin-dotnet-source,:gremlin-dotnet-tests -DskipIntegrationTests=false

VOTE+1

jorgebay

Awesome! I've added 2 small nits below.

jorgebay · 2018-03-16T09:24:36Z

gremlin-dotnet/src/Gremlin.Net/Process/Traversal/P.cs

        public static P Within(params object[] args)
        {
-            return new P("within", args);
+            if (args.Length == 1 && args[0].GetType().GetInterfaces().Contains(typeof(ICollection)))


Can be simplified to args.Length == 1 && args[0] is ICollection<object>

jorgebay · 2018-03-16T09:29:26Z

gremlin-dotnet/src/Gremlin.Net/Process/Traversal/P.cs

+
+        private static T[] ToGenericArray<T>(ICollection<T> collection)
+        {
+            if (collection == null)


We can express the logic in this method using null-conditional and null-coalescing operators:

private static T[] ToGenericArray<T>(ICollection<T> collection) { return collection?.ToArray() ?? new T[0]; }

spmallette · 2018-03-16T12:05:46Z

@jorgebay i implemented your suggestions - thanks

jorgebay · 2018-03-16T12:07:31Z

VOTE +1

FlorianHockmann

Nice, this removes the last ignored tests on tp32

I also just have two nits

FlorianHockmann · 2018-03-16T13:20:53Z

gremlin-dotnet/src/Gremlin.Net/Process/Traversal/P.cs

        {
-            return new P("within", args);
+            if (args.Length == 1 && args[0] is ICollection<object>)
+                return new P("within", ToGenericArray((ICollection<object>) args[0]));


This (ICollection<object>) cast can even be removed like this:

if (args.Length == 1 && args[0] is ICollection<object> collection) return new P("without", ToGenericArray(collection));

FlorianHockmann · 2018-03-16T13:22:45Z

gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Gherkin/GherkinTestRunner.cs

-            new Dictionary<string, IgnoreReason>
-            {
-                {"g_V_hasIdXwithinXemptyXX_count", IgnoreReason.PWithinWrapsArgumentsInArray},
-                {"g_VX1X_out_aggregateXxX_out_whereXnotXwithinXaXXX", IgnoreReason.PWithinWrapsArgumentsInArray}


Please also remove the enum value here: https://github.com/apache/tinkerpop/blob/tp32/gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Gherkin/IgnoreException.cs#L60

dkuppitz · 2018-03-16T17:44:55Z

VOTE +1 (pending Florian's 2 comments)

spmallette · 2018-03-19T13:04:20Z

@FlorianHockmann I rebased and now the tests are failing (with and without your suggested changes) - i assume this has something to do with the lambda work that just merged - any ideas?

Failures:
1) g_V_hasIdXwithinXemptyXX_count: Failed
Xunit.Sdk.ContainsException: Assert.Contains() Failure
Not found: 6
In value:  Object[] [0]
   at Xunit.Assert.Contains[T](T expected, IEnumerable`1 collection, IEqualityComparer`1 comparer)
   at Gremlin.Net.IntegrationTest.Gherkin.CommonSteps.AssertResult(String characterizedAs, DataTable table) in /home/smallette/git/apache/incubator-tinkerpop/gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Gherkin/CommonSteps.cs:line 188
2) g_V_notXhasIdXwithinXemptyXXX_count: Failed
Xunit.Sdk.ContainsException: Assert.Contains() Failure
Not found: 0
In value:  Object[] [6]
   at Xunit.Assert.Contains[T](T expected, IEnumerable`1 collection, IEqualityComparer`1 comparer)
   at Gremlin.Net.IntegrationTest.Gherkin.CommonSteps.AssertResult(String characterizedAs, DataTable table) in /home/smallette/git/apache/incubator-tinkerpop/gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Gherkin/CommonSteps.cs:line 188

FlorianHockmann · 2018-03-19T13:32:05Z

gremlin-dotnet/glv/P.template

-            if (args.Length == 1 && args[0] is ICollection<object>)
-                return new P("<%= method %>", ToGenericArray((ICollection<object>) args[0]));
+            if (args.Length == 1 && args[0] is ICollection<object> collection)
+                return new P("without", ToGenericArray(collection));


I don't think the method should be hard-coded as without here 😄

I just debugged one of the failing tests and was really confused when I saw P.without() in the Bytecode although the scenario only uses P.within(), but good that we now have such a good test coverage with the Gherkin features that we immediately find slips like this one 😅

Nice catch! I've looked at it and missed it completely :)

hahaha....oops - copy/paste. thanks-that was dumb on my part

jorgebay reviewed Mar 16, 2018

View reviewed changes

asfgit force-pushed the TINKERPOP-1920 branch from 0651948 to a4f20a2 Compare March 16, 2018 11:47

FlorianHockmann reviewed Mar 16, 2018

View reviewed changes

asfgit force-pushed the TINKERPOP-1920 branch from a4f20a2 to 75490b1 Compare March 19, 2018 13:02

FlorianHockmann reviewed Mar 19, 2018

View reviewed changes

TINKERPOP-1920 Fixed P.within/without() handling for collections

8c87fcf

asfgit force-pushed the TINKERPOP-1920 branch 2 times, most recently from cfc04a8 to 8c87fcf Compare March 19, 2018 14:17

asfgit merged commit 8c87fcf into tp32 Mar 19, 2018

jorgebay mentioned this pull request Mar 20, 2018

TINKERPOP-1865 Run Gremlin.Net tests with GraphSON 3.0 #820

Merged

asfgit deleted the TINKERPOP-1920 branch April 14, 2018 15:44

Conversation

spmallette commented Mar 15, 2018

Uh oh!

jorgebay left a comment

Choose a reason for hiding this comment

Uh oh!

jorgebay Mar 16, 2018

Choose a reason for hiding this comment

Uh oh!

jorgebay Mar 16, 2018

Choose a reason for hiding this comment

Uh oh!

spmallette commented Mar 16, 2018

Uh oh!

jorgebay commented Mar 16, 2018

Uh oh!

FlorianHockmann left a comment

Choose a reason for hiding this comment

Uh oh!

FlorianHockmann Mar 16, 2018

Choose a reason for hiding this comment

Uh oh!

FlorianHockmann Mar 16, 2018

Choose a reason for hiding this comment

Uh oh!

dkuppitz commented Mar 16, 2018

Uh oh!

spmallette commented Mar 19, 2018

Uh oh!

FlorianHockmann Mar 19, 2018

Choose a reason for hiding this comment

Uh oh!

jorgebay Mar 19, 2018

Choose a reason for hiding this comment

Uh oh!

spmallette Mar 19, 2018

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants